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

Fix UsersResponderV1 to return None and not an exception when user not found #297

Closed
subotic opened this issue Oct 27, 2016 · 7 comments
Closed
Assignees
Labels
bug something isn't working
Milestone

Comments

@subotic
Copy link
Collaborator

subotic commented Oct 27, 2016

When a user is not found, then the UsersResponderV1 should return None and not a NotFoundException which is translated to into a 404 status code .

@subotic subotic added the bug something isn't working label Oct 27, 2016
@subotic subotic added this to the Beta Release milestone Oct 27, 2016
@subotic subotic self-assigned this Oct 27, 2016
@benjamingeer
Copy link

I thought we decided that NotFoundException and the 404 are correct?

benjamingeer pushed a commit that referenced this issue Nov 8, 2016
- Add a button for getting the creator and creation date of a value (#309, WIP)
- Change UsersResponderV1 not to return an Option, relying on exceptions instead (#297).
@benjamingeer
Copy link

In #315 I've changed UsersResponderV1 so it doesn't use Option anymore, and just throws an exception if a requested user is not found. I also changed Authenticator to handle this change. Please let me know if this looks OK to you.

@subotic
Copy link
Collaborator Author

subotic commented Nov 9, 2016

Yes, this is fine for now. I opened this issue before and completely forgot about it.

I say for now, because I thing that the responder should return an option, and the route should then throw an exception, if it want's. This way, we decouple the routes behaviour from the responder.

Generally speaking, I see two different use cases for a responder:

(1) Requesting something through the route, which should return Not Found when the IRI is not correct,

(2) Internal use, by asking one responder from another for something. The response in this case should be preferably None.

But to support both cases in this manner, we would need to refactor RouteUtil a bit, so that the result from the responder is returned to the calling route, and then the route needs to throw the exception or create a HttpResponse in a separate step.

@benjamingeer
Copy link

Thanks for looking at this. Currently when responders talk to each other, they can get NotFoundException if they ask for something (a resource, a value) that doesn't exist. This simplifies things for them, because if a responder gets a NotFoundException from another responder, the normal case is that the exception is just returned to the client. Since that's done automatically, there's no need for the (receiving) responder or the route to do anything about it. Currently I don't think we have any cases where a responder asks for something from another responder and it's OK if it doesn't exist.

@subotic
Copy link
Collaborator Author

subotic commented Nov 9, 2016

Currently I don't think we have any cases where a responder asks for something from another responder and it's OK if it doesn't exist.

Up until now ;-) But since the user-management2 branch is in constant flux, this can change. I will keep you posted.

@benjamingeer
Copy link

OK :) It's not hard to change this if we need to.

@subotic
Copy link
Collaborator Author

subotic commented Nov 10, 2016

I thought about this a bit more. If this is needed only in rare cases, then we could probably write a method that turns the result/exception into an Option. This way, we don't need to refactor the responders themselves.

Also, I had already made these changes in the user-management2 branch, but your refactoring of the Authenticator is much nicer and better :-)

benjamingeer pushed a commit that referenced this issue Nov 15, 2016
#315)

* fix (salsah): Fix the creation of multiple linking properties (#277).

- Add a button for getting the creator and creation date of a value (#309, WIP)
- Change UsersResponderV1 not to return an Option, relying on exceptions instead (#297).

* feature (salsah): Finish implementing #309.

* test (add biblio-data): add biblio-data to the test data in the browser tests

* fix (webapi): Use FILTER NOT EQUALS instead of MINUS in changeLink.scala.txt for better performance on Fuseki.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug something isn't working
Projects
None yet
Development

No branches or pull requests

2 participants