getXY throws exception if XY not exists

[Spenhouet]

It seems that get operations (not sure if this is a general behaviour) throw an exception if the requested object doesn't exist. I don't think this is good and in my opinion the get request should just return null or Optional.empty if the requested object doesn't exists.

The problem here is the following:
If I want to get an object that may or may not already exist. If it exists it should just be returned but if it doesn't exist it should be created and then returned.
In my eyes this is a common use case.

If for example I want to get a specific group that may or may not exists but that definitely should be returned.

I can try to get the group groupApi.getGroup(...) but if it doesn't exist this will throw an exception so something like the following doesn't work:

Optional<Group> groupOptional = Optional.ofNullable(groupApi.getGroup(...));
if (groupOptional.isPresent())
    return groupOptional.get();

I can just try to create the group groupApi.addGroup(...) but if the group already exists this will also throw an exception.

The only thing that can be done is something like the following but that is in my eyes very bad design:

try {
    return groupApi.getGroup(...);
} catch (GitLabApiException e) {
    if (e.getHttpStatus() == 404)
        return groupApi.addGroup(...);

    throw e;
}

I think throwing a exception when someone tries to create something that already exists is a correct behaviour but throwing an exception if the requested object doesn't exists isn't good.
Just returning null or Optional.empty would make much more sense. Returning an Optional also would force everyone to check if the request returned anything.

I did see that the GitLab API does throw a 404 Group Not Found exception. But I'm not sure if it makes sense to propagate this error further into the Java API.

@gmessner What do you think about this change request? Does this behaviour has another reason than that the GitLab API does it like this? Is there a better way than I'm doing it?

[gmessner]

All getXyz() methods in GitLab4J act this way. Since GitLab returns a 404, how would GitLab4J know whether the 404 is the result of some sort of object not being found, or that the API endpoint was not found because of an issue on the server, etc. ? So, it must be treated as an exception.

Additionally, there are many reasons that a getXyz() method could fail other than a 404, there could be a communication issue, auth issue, etc. In my opinion, any time an application is using a remote API it is good practice to wrap the calls in a try block.

This behavior has been this way since the very first release of GitLab4J, nearly 4 years ago, and it would impact too many users of GitLab4J that are expecting the exception, causing them to have to modify their code to accommodate this major change.

That being said, one possibility would be to add findXyz(...) methods to GitLab4J that would behave in the way that you describe. One problem is that the Optional class is only available on Java 1.8+, I would need to ask the user's of GitLab4J whether having a Java 1.8 requirement would impact them (currently it is Java 1.7+). The other option would be to return a GitLab4J class from the findXyz() methods that would behave like he Java 1.8 Optional class.

[gmessner]

@Spenhouet
Can I close this or do you wish to discuss the findXyz() proposal?

[Spenhouet]

@gmessner Sorry for my late response.

You are right, I somehow didn't think about the fact that 404 could come from many other reasons than that the resource isn't available.
On that point I'm just wondering that the error reply actually contains the string XYZ Not Found. So there should be the information if the 404 came from the missing resource or some other issue. But handling the 404-event different based on the error message string doesn't sound like a good solution either.

I like the idea of the find method since it wouldn't impact existing GitLab4J usages. But it should use the Optional provided by Java. On the one side I feel like everyone is using Java 1.8 at that time but on the other side I don't feel comfortable to ask you to invest so much effort. If you think a findXyz(...) method would be a great addition I would be happy to see it implemented with Java 1.8 Optionals.

Thank you for your effort 👍

[gmessner]

Since Oracle stopped publically supporting Java 7 in April of 2015 (nearly 3 years ago) it is time that GitLab4J moves on to Java 8, especially considering that Java 9 is the current release. So the using Java 8 Optional will not be a problem.

I need to take a thorough look at how many findXyz() methods would need to be implemented, but I have some thoughts that I would like feedback on:

• Reduce the scope of work by only adding findXyz() methods for getXyz() methods that return a single instance (not List<> or Pager<>)
• Should the naming pattern be findXyz() or getOptionalXyz() ? I lean towards getOptionalXyz() as it expresses the intent better, and there are already findXyz() methods in GitLab4J
• Support orElseThrow() providing the GitLabApi instance as the exceptionSupplier (Supplier). GitLabApi would keep track of the exception belonging to an instance of Optional and throw the actual exception. You could also look up the exception on the GitLab4J instance without throwing an exception

[Spenhouet]

I think implementing Optionals only for getXyz() methods that return a single instance makes sense and is sufficient.
Yes, getOptionalXyz() is a very clear naming schema. Sounds good to me.

Your last point sounds interesting but I'm not sure I understand how you would implement that?

[gmessner]

After further thought the orElseThrow() idea does not work, however, having the API keep a smart cache of Map<Optional, GitLabApiException> would allow you to fetch the actual exception that caused null if there was one, something like this:

Optional<Project> optional = gitLabApi.getProjectApi().getOptionalProject(1);
GitLabApiException glae = gitLabApi.getOptionalException(optional);

I'm just pondering ideas at this time, this might be useful but who knows?

[Spenhouet]

I'm not sure if this is that important to be able to do. The Map<> solution is potentially a problem since it would need to be emptied on times to not only increase. It should also only contain entries if there was an exception. If not getOptionalException should also return a Optional. But again, I'm not sure this is actually needed. With getOptionalXyz() you already would need to handle the two cases if something was returned or not. If in case of nothing returned someone would handle the exception, he could just use the original implementation with a try catch.

[gmessner]

I called it a smart cache as it would remove instances of Optional that are no longer used by the application (a week reference). And yes it would only contain Optional instances that were the result of an exception. This would allow one to not wrap things in a try catch block and if the value of the Optional was null, you could still get the original exception that caused the value to be null, for logging, etc.. So after further thought, I'm actually thinking this:

1. There would be a singleton of the Supplier implementation for the exceptions
2. The exception Supplier would provide the smart cache for the Optional/Exception entries
3. The application code could make use of GitLabApi.orElseThrow(optional) or call GitLabApi.getOptionalException(optional) directly

This would look something like this:

Optional<Project> optional = gitLabApi.getProjectApi().getOptionalProject(1);
Project project = GitLabApi.orElseThrow(optional);

If you wanted to get the causing exception:

GitLabApiException glae = GitLabApi.getOptionalException(optional);

[Spenhouet]

Sounds flexible. Could be use full for many use cases. I like it 👍

[gmessner]

@Spenhouet
I have added getOptionalXyz() support to the following APIs:

• GroupApi
• ProjectApi
• UserApi

I will implement the other APIs with getOptionalXyz() methods after you have an opportunity to review what I've done. I would like to ask one favor, could you take a look at the section in README.md on the Java 8 Optional support, if you'd like to add or edit the section, please do and submit a PR.