Ap 8726 - Update library to support all current APIs.#11
Ap 8726 - Update library to support all current APIs.#11tcochran317 wants to merge 6 commits intomasterfrom
Conversation
AP-8726 formatting fixes.
|
Also found and fixed a few API issues while testing this: https://github.com/apruve/apruve/pull/3364 |
simonbtomlinson
left a comment
simonbtomlinson
left a comment
There was a problem hiding this comment.
I'll finish looking at this tomorrow, but I pretty much understand what's going on and it seems good so far, with minor questions.
| public String getName() { | ||
| return name; | ||
| } | ||
| public void setName(String name) { |
There was a problem hiding this comment.
Why would anyone ever need to set a name? Our api doesn't support creating users.
Or is this required by the serialization library?
There was a problem hiding this comment.
It's a Java thing. If there's no public setter method nothing outside of the class (like a JSON deserializer) can modify the property.
|
|
||
| protected static String doMarshalTest(Object obj) { | ||
| String json = JsonUtil.getInstance().toJson(obj); | ||
| System.out.println(obj.getClass().getName() + ":" + json); |
There was a problem hiding this comment.
Do we want to be printing stuff during tests?
|
|
||
| public class ApruveModelTestHelper { | ||
|
|
||
| protected static String doMarshalTest(Object obj) { |
There was a problem hiding this comment.
Is the point here just to make sure it doesn't throw any errors? Or are we missing something that tests that it marshals back to a reasonable representation?
There was a problem hiding this comment.
Yes, it's looking for configuration problems which would throw exceptions from the serialization framework. Though I realize in trying to DRY out the code I inadvertently switched to a serialization method that intentionally fails without throwing an exception. Whoops.
simonbtomlinson
left a comment
simonbtomlinson
left a comment
There was a problem hiding this comment.
Looks good! The mapper isn't threadsafe is the only thing I can think of, but I really doubt that'll come up.
|
Actually, ObjectMapper is thread safe so long as threads don't change the configuration of the mapper (which this implementation does not allow). https://stackoverflow.com/questions/3907929/should-i-declare-jacksons-objectmapper-as-a-static-field |
3 participants
No description provided.