Rdf iteration for mutation3 #137
Conversation
| try { | ||
| final FedoraResource resource = | ||
| nodeService.getObject(session, path); | ||
| final FedoraResource resource = nodeService.getObject(session, path); |
There was a problem hiding this comment.
If the arg path does not exist, the top-level exception handler will not return the wanted 404, will it?
There was a problem hiding this comment.
Looking at the unedited, it never could have. +1 to our integration tests for catching that boo-boo! Wait, no, they didn't. I'll add it in when I get a chance.
There was a problem hiding this comment.
Are you saying
- that the path can never be bad? or
- that the response will be 404? or
- that it did not work right before and still will not work right? or
- something else?
There was a problem hiding this comment.
The third. And what's more, we didn't know it and it was only because you happened to review a change to it that we found out.
There was a problem hiding this comment.
If you are not inclined to fix it now, could you please add a ticket at least, and we can push this branch in.
Thanks.
There was a problem hiding this comment.
Now I'm not sure. Won't the PathNotFoundExceptionMapper handle this? Is that why we never saw a problem before?
There was a problem hiding this comment.
A quick test indicates you are correct:
curl -v http://localhost:8080/rest/junk/fcr:versions
Let's ship it.
There was a problem hiding this comment.
Okay, but I stand by every word of my snarky ranting about i-tests.
|
Nice job with the Futures and Callback. |
|
It's not obvious what happens there, especially in a transactional context. IOW, it works, but it doesn't help people understand itself. There's probably a factoring there to bring some of that stuff out into the type system where it can be more effectively tested/documented. |
|
Resolved with: |
Last interim PR for this saga before Austin, where we'll refactor the epic.