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

getOptionalXXX() method can return false Optional.empty() #1095

Open
amitt00r opened this issue Feb 27, 2024 · 2 comments
Open

getOptionalXXX() method can return false Optional.empty() #1095

amitt00r opened this issue Feb 27, 2024 · 2 comments

Comments

@amitt00r
Copy link

amitt00r commented Feb 27, 2024

We recently faced one issue where below method returned false Optional.empty() for a file which did exist. On checking the code found issue with below code in RepositoryFileApi class.

  public Optional<RepositoryFile> getOptionalFileInfo(Object projectIdOrPath, String filePath, String ref) {
        try {
            return Optional.ofNullable(this.getFileInfo(projectIdOrPath, filePath, ref));
        } catch (GitLabApiException var5) {
            return GitLabApi.createOptionalFromException(var5);
        }
    }

Issue:
Method GitLabApi.createOptionalFromException(var5) above doesn't do status code checks on GitLabApiException before returning Optional.empty(). This method is used across all getOptionalXXX() methods in all Apis and this means all getOptionalXXX() methods in all repositories has this issue.

Expected Behaviour:
This method should first check on 'httpStatus' == 404 to ensure that file indeed doesn't exist on server and then return optional.empty(). All other exceptions should be rethrown wrapped as Suitable Exception.

@jmini
Copy link
Collaborator

jmini commented Feb 28, 2024

Interesting finding.

The problem I see with "rethrow": all the getOptionalXXX() methods are not throwing any Exception. So we should maybe wrap it into a Runtime exception. Not sure if it makes sense or not.

I looked at the code and I am also not convinced by the mechanism of storing the exception in a map.

@amitt00r
Copy link
Author

Yep, I found it later all methods returning Optional have this issue, all use same method GitlabApi.createOptionalFromException() which never throws exception no matter what the status code is.

@amitt00r amitt00r changed the title RepositoryFileApi's getOptionalFileInfo method can return false Optional.empty() getOptionalXXX() method can return false Optional.empty() Mar 11, 2024
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

No branches or pull requests

2 participants