#784 - Introduce UBER+JSON mediatype. #785
Conversation
gregturn
force-pushed
the
feature/uber4
branch
from
0aedb79 to
8da3a85
Compare
Formatting. Extracted conversion methods.
Tested and fixed NPE with empty pojo.
Tested and fixed NPE with empty pojo.
Tested and fixed NPE with empty pojo.
Tested and fixed NPE with empty page meta data.
There was a problem hiding this comment.
I have couple of complains:
-
the copyright headers need updating.
-
empty data elements can trigger various NPEs. I added tests and fixes in extra commits, but aren't sure at all if the behavior is intended. But I think the JSON is legit according to the spec so we at least shouldn't fail with an NPE. See also my comment below about an alternative handling of the
nullvalues. -
There isn't any handling of error values which is part of the UBER+JSON spec. I wonder if we should add something for that too.
| .orElse(null); | ||
| } | ||
|
|
||
| public void setTemplated(boolean __) { |
There was a problem hiding this comment.
I tried removing this and the tests still worked, so this is either superfluous or we are missing a test.
There was a problem hiding this comment.
Added a test case and applied similar patch as 8bf01c3.
There was a problem hiding this comment.
Fixed copyright headers.
There was a problem hiding this comment.
Opened #786 to track a comprehensive approach to error handling and hypermedia.
| * @param object | ||
| * @return | ||
| */ | ||
| static UberDocument toUberDocument(final Object object) { |
There was a problem hiding this comment.
This looks like a leftover and it hasn't any usage. Can we remove it?
| */ | ||
| public List<UberData> getData() { | ||
|
|
||
| if (this.data == null || this.data.isEmpty()) { |
There was a problem hiding this comment.
This is the reason for a couple of null-checks we need in the deserializer. I wonder if we can achieve the same by some Jackson annotation (https://stackoverflow.com/a/29928205/66686)
The same is true for other properties that return null in special cases.
I removed the logic and no test failed, so I think we at least need some additional tests.
There was a problem hiding this comment.
I have commented out both the getData() and the getRel() methods, and verified that everything works. This was after adding another test case and verifying it hits this. Conclusion: we must be covering the "null" situation properly elsewhere, making this rendundant.
|
Thanks @schauder and @dschulten for the time and effort it has taken to land this feature on the master branch. |
|
And of course @odrotbohm. :) |
Based on https://rawgit.com/uber-hypermedia/specification/master/uber-hypermedia.html.