Repository navigation
Allow passing POJOs as field values throughout API. - #76
Conversation
All of our parsing code now accepts POJOs meaning you can pass them: * As values in a Map<> passed to set(). * As values to a update() call. * As elements in an array transform operation. * As values in query field filters. Also removed set(Map<>) overloads since they are now redundant with set(Object).
mikelehen
left a comment
mikelehen
left a comment
There was a problem hiding this comment.
FYI-
- This feature comes with a code savings of 78 lines!
- This hasn't finished going through API review so no rush on the review (you can hold off if you want).
| reason = reason + " (found in field '" + path.toString() + "')"; | ||
| } | ||
| return new RuntimeException(reason); | ||
| return new IllegalArgumentException(reason); |
There was a problem hiding this comment.
Some errors (in particular trying to use an Array instead of a List) are now getting caught in the POJO mapper instead of our UserDataConverter. To keep the exception type consistent, I CustomClassMapper to throw IllegalArgumentExceptions (which is more correct anyway).
| fail("wrap should have failed"); | ||
| } catch (IllegalArgumentException e) { | ||
| assertNotEquals(-1, e.getMessage().indexOf("use a List instead")); | ||
| assertNotEquals(-1, e.getMessage().indexOf("use Lists instead")); |
There was a problem hiding this comment.
This error is now coming from CustomClassMapper and so the wording changed slightly.
schmidt-sebastian
left a comment
schmidt-sebastian
left a comment
There was a problem hiding this comment.
LGTM. I left one optional suggestion that can help you make this PR longer (impact!).
| // Convert POJOs | ||
| Object converted = CustomClassMapper.convertToPlainJavaTypes(input); | ||
|
|
||
| if (source == UserData.Source.Set && !(converted instanceof Map)) { |
There was a problem hiding this comment.
How excited do you feel about moving this assert (and maybe the if-check in 203) to parseMergeData and parseSetData?
There was a problem hiding this comment.
I made a convertAndParseDocumentData() helper instead. Does that work? This added 3 lines, but I found some test comments to fix up which ended up removing 7 lines, so we're okay.
|
/retest |
|
/test smoke-tests-debug |
|
/test smoke-tests-debug |
|
/test smoke-tests-debug |
|
/retest |
* Added `T get(..., Class<T>, ...)` overloads for extracting fields as a custom class (POJO) type.
|
/retest |
|
/test connected-check |
5 participants
All of our parsing code now accepts POJOs meaning you can pass them:
Also removed set(Map<>) overloads since they are now redundant with set(Object).