1003: Implement JSONObject.fromJson() with unit tests #1006
Conversation
|
@sk02241994 I won't have time to review in depth until later this week. My initial take is that fromJson() is long overdue, glad to see someone submit an implementation, thanks for that! For now, it will be better to get everything working and released with just the default options, so JSONBuilder can wait for a future PR. Also, all of the new SonarQube issues will need to be addressed, or marked false-positive if appropriate. |
| public <T> T fromJson(Class<T> clazz) { | ||
| try { | ||
| T obj = clazz.getDeclaredConstructor().newInstance(); | ||
| if (this.builder == null) { |
There was a problem hiding this comment.
Per earlier comment, please omit JSONBuilder from the initial implementation.
There was a problem hiding this comment.
@stleary Can you give me an idea on how you want that implementation to be done?
I went this route since there are some types like LocalDate/Time or UUID etc, which require some customization or processing before being converted.
Or are we avoiding converting to those types for now?
There was a problem hiding this comment.
@sk02241994 Sure, Take the behavior of a default JSONBuilder instance and hardcode it in the fromJson() functionality. Ideally, you might be able to make a case for keeping JSONBuilder, but removing it from the API for now. But only do this if you can keep the new code as easy to understand as possible. Right now, I don't think that is the case. More inline comments might help a bit. Developers often have difficulty commenting their code. A useful way to think of it is to imagine you are in a code review, and you are asked to explain why you did something a certain way, or what is the purpose of a class or local variable. Then just turn your explanation into a comment.
| assertEquals(customClass, compareClass); | ||
| } | ||
|
|
||
| public static class CustomClass { |
There was a problem hiding this comment.
Each CustomClass* should be in its own file in the org.json.junit/data directory
sk02241994
force-pushed
the
feature-1003
branch
2 times, most recently
from
4f67189 to
f0b7d1a
Compare
|
@stleary Sorry for the delay. |
|
@sk02241994 Can you do the implementation without adding any new classes? I understand that this will limit the types that fromJson() can support. The JavaDocs for the new methods should explain the limitations. |
- Moving the CustomClass to data folder. - Removing JSONBuilder.java - Moving the implementation of JSONBuilder to JSONObject.
- Resolving Sonar cube issues.
sk02241994
force-pushed
the
feature-1003
branch
from
f0b7d1a to
9adea9e
Compare
|
@stleary Sorry for the delay. I have updated the code to deserialize limited number of classes. Only those which are present in the JSONObject can be deserialized. Since we need to TypeConverter and InstanceCreatory interface, I am keeping those but changing the access type to be default, so no one can access those. |
|
@sk02241994 The code cannot be accepted in its current form. |
|
|
@stleary I have updated the code there are some sonar issues which are irrelevant to the code changes I have made. |
|
Thanks @sk02241994, the changes look good, and I should be able to complete the review today. |
|
What problem does this code solve? Does the code still compile with Java6? Risks Changes to the API? Will this require a new release? Should the documentation be updated? Does it break the unit tests? Was any code refactored in this commit? Review status Starting 3-day comment window |
stleary
added
Approved - 3-day window
and removed
Code review action items
Approved - 3-day window
labels
2 participants
#1003
Add support for JSON to POJO deserialization via JSONBuilder
Introduced JSONBuilder class:
Provides customizable mappings for type conversions (Function<Object, ?>) and collection instantiation (Supplier<?>), enabling extensible JSON deserialization logic.
Enhanced JSONObject:
Added support for converting JSON into Java POJOs using a new method: fromJson(Class).
Supports nested objects, collections (e.g., List), and custom type mappings via JSONBuilder.
Optional JSONBuilder can be passed via constructor or setter.
Added comprehensive unit tests in JSONObjectTest:
Covers deserialization of primitive types, nested classes, collections, and parameterized generics (e.g., List<List>).
Includes tests for custom type mapping (e.g., LocalDateTime).