Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Team ID fix in Schedule and Policy importer #57

Merged

Conversation

sghosh4-atlassian
Copy link
Contributor

@sghosh4-atlassian sghosh4-atlassian commented Jun 28, 2023

This fixes the following issues during import -

  • Updating the schedule with rotation information. Currently it throws the error - Error at updating Schedule Group A_schedule.{"message":"Team with 'id' [{old teamId}] does not exist"}
  • Adding a alert policy having a responder team which is different from owner team. Currently it throws the error - Error at adding Policy route to team b. {"message":"Responder does not exist with id:{old teamId}"}

String[] files = getFileListOf(teamsDirectory);
if (files == null || files.length == 0) {
logger.warn("Warning: {} is empty", teamsDirectory);
return null;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will it be possible to have Optional rather than String which would decrease the chance of NPE? If it is not a large task.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since the current Java version for this project is set as 1.6, we won't be able to use any Java 8 features. Hence can't really use Optional without updating the java version.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we create out own Optional implementation something similar to :

public class Optional<T> {
    private final T value;

    private Optional(T value) {
        this.value = value;
    }

    public static <T> Optional<T> of(T value) {
        if (value == null) {
            throw new NullPointerException("Value cannot be null");
        }
        return new Optional<T>(value);
    }

    public static <T> Optional<T> ofNullable(T value) {
        return new Optional<T>(value);
    }

    public T get() {
        if (value == null) {
            throw new IllegalStateException("Value is not present");
        }
        return value;
    }

    public boolean isPresent() {
        return value != null;
    }

    public T orElse(T defaultValue) {
        return value != null ? value : defaultValue;
    }
}
```       

Usage:

```java
public class Main {
    public static void main(String[] args) {
        Optional<String> stringOptional = Optional.ofNullable(getStringValue());

        if (stringOptional.isPresent()) {
            System.out.println("Value: " + stringOptional.get());
        } else {
            System.out.println("Value not present");
        }
    }

    private static String getStringValue() {
        // Your logic to return a string or null
        return "Hello, World!";
    }
}

Copy link
Contributor Author

@sghosh4-atlassian sghosh4-atlassian Jul 18, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, something like this can be created. But here also we need to assign null ( Optional.ofNullable(null) ) to create empty objects unlike Java 8 Optional class where we can use Optional.empty().

So the usage of null still can not be completely avoided.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added a custom OptionalUtil

return Arrays.stream(fileName.split("-")).findFirst().orElseGet(null);
}
}
return null;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

May be if we are using Optional we can send Optional.Empty()

protected void updateTeamIds(ScheduleConfig entity) {}
protected void updateTeamIds(ScheduleConfig entity) throws Exception {
Map<String, String> teamIdMap = new TeamIdMapper(rateLimitManager).getTeamIdMap();
TeamMeta ownerTeam = entity.getSchedule().getOwnerTeam();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we forsee a case where entity.getSchedule() can be null and this leads to NPE?

if(responder.getType() == Responder.TypeEnum.TEAM) {
File teamsDirectory = new File(backupRootDirectory + "/teams/");
String responderTeamName = BackupUtils.getTeamNameFromId(teamsDirectory, responder.getId());
if (responderTeamName != null){
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i have a clarifying question is responderTeamName is string? If yes, can we use StringUtils.empty(responderTeamName) otherwise we are good.

@@ -46,6 +47,20 @@ public static String readFile(String fileName) throws IOException {

return sb.toString();
}
public static String getTeamNameFromId(File teamsDirectory, String teamId) {
String[] files = getFileListOf(teamsDirectory);
Copy link
Contributor

@trabab trabab Jul 14, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can you fix getFileListOf function to return empty list instead of null.

@@ -46,6 +47,20 @@ public static String readFile(String fileName) throws IOException {

return sb.toString();
}
public static String getTeamNameFromId(File teamsDirectory, String teamId) {
String[] files = getFileListOf(teamsDirectory);
if (files == null || files.length == 0) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we can get rid of the null check if we address the above comment.

@sghosh4-atlassian sghosh4-atlassian merged commit 356ab17 into master Jul 24, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants