issue#2436: Throw exception when registering adapter for Object or JsonElement #2479

sachinp97 · 2023-08-25T19:28:05Z

Purpose

Closes #2436 to throw IllegalArgumentException for certain Types

Description

Implementation includes a check of Type of an Object class or assignable from JsonElements (i.e. subclass or self).
It seems following use cases of Parameterized Type failed after latest changes.
Additional Fix also excludes Parameterized Type that cover 7 other test cases.

Checklist

[ x] New code follows the Google Java Style Guide
[ x] If necessary, new public API validates arguments, for example rejects null
[ x] New public API has Javadoc
- [x ] Javadoc uses @since $next-version$
  ( $next-version$ is a special placeholder which is automatically replaced during release)
[ x] If necessary, new unit tests have been added
- [ x] Assertions in unit tests use Truth, see existing tests
- [ x] No JUnit 3 features are used (such as extending class TestCase)
- [ x] If this pull request fixes a bug, a new test was added for a situation which failed previously and is now fixed
[ x] mvn clean verify javadoc:jar passes without errors

… to register adapter for Object or JsonElement

…cases and fix for 7 test cases of Parameterized Type

Marcono1234

Thanks a lot for your changes! They look pretty good.

Could you please edit the title of your pull request? I think GitHub will otherwise use it by default as commit message. For example what about the following?

Throw exception when registering adapter for Object or JsonElement

I have added a few review comments; some are only related to formatting and wording (feel free to use a better wording than what I proposed). Though in general feel free to consider my review comments mostly as suggestions since I am not a direct member of this project.

UserGuide.md

gson/src/main/java/com/google/gson/GsonBuilder.java

gson/src/test/java/com/google/gson/GsonBuilderTest.java

…achinp97/gson into feature/issue#2436_enhancement

sachinp97 · 2023-08-28T23:31:44Z

Thanks a lot for your changes! They look pretty good.

Could you please edit the title of your pull request? I think GitHub will otherwise use it by default as commit message. For example what about the following?

Throw exception when registering adapter for Object or JsonElement

I have added a few review comments; some are only related to formatting and wording (feel free to use a better wording than what I proposed). Though in general feel free to consider my review comments mostly as suggestions since I am not a direct member of this project.

gson/src/main/java/com/google/gson/GsonBuilder.java

gson/src/test/java/com/google/gson/GsonBuilderTest.java

Marcono1234

Thanks for your changes already!

You can change the title of the pull request by clicking the "Edit" button at the top right of the pull request.

Also, there are a few comments still unresolved, unfortunately the GitHub UI has the bad habit of hiding them regardless of whether they are resolved or not...

You can also see all unresolved comments on the "Files changes" tab under "Conversations", see also the GitHub documentation.

…or message.

…achinp97/gson into feature/issue#2436_enhancement

sachinp97 · 2023-09-19T14:50:26Z

@Marcono1234 @eamonnmcmanus , I have pushed the changes as per review.
Please have a final review before the merge.

Thanks for great review and comments!

eamonnmcmanus

This is looking good. I'm running it against all of Google's internal tests just to make sure there are no unexpected problems.

A couple of small things. And also, could you update the CL with the latest changes from HEAD?

gson/src/main/java/com/google/gson/GsonBuilder.java

Marcono1234

Thank you very much for your changes!

In addition to Éamonn's comments I added a few small additional comments but besides that the pull request looks fine to me as well.
If you want you can also directly apply the suggested changes from the GitHub UI. (Also don't worry too much about the commit messages; the commits will be squashed into a single commit here anyways when merged.)

gson/src/main/java/com/google/gson/GsonBuilder.java

gson/src/test/java/com/google/gson/GsonBuilderTest.java

Co-authored-by: Marcono1234 <[email protected]>

sachinp97 · 2023-09-29T01:15:28Z

Thanks @eamonnmcmanus and @Marcono1234 again for final reviews, please confirm so I can merge and close it.

eamonnmcmanus

If @Marcono1234 is also OK with these changes we can merge them.

sachinp97 · 2023-10-02T23:07:16Z

I don't have a write access to this repository, @Marcono1234 can you suggest if we are good and anyone can merge the PR?

Marcono1234

Yes for me this is fine as well, thanks a lot for your work on this!

…onElement (google#2479) * Code changes and tests for google#2436 to throw exception when trying to register adapter for Object or JsonElement * google#2436 - Updates to User guide & comments to indicate exception cases and fix for 7 test cases of Parameterized Type * google#2436 - Fixes as per the review comments. * google#2436 - Refactored as per latest review comments + throwing error message. * google#2436 - added a clarifying comment in a positive test case. * google#2436 - formatting and minor changes as per review. * Update gson/src/main/java/com/google/gson/GsonBuilder.java Co-authored-by: Marcono1234 <[email protected]> * Update gson/src/test/java/com/google/gson/GsonBuilderTest.java Co-authored-by: Marcono1234 <[email protected]> * Update gson/src/test/java/com/google/gson/GsonBuilderTest.java Co-authored-by: Marcono1234 <[email protected]> --------- Co-authored-by: Sachin Patil <[email protected]> Co-authored-by: Marcono1234 <[email protected]>

Sachin Patil and others added 3 commits August 21, 2023 12:31

Code changes and tests for google#2436 to throw exception when trying…

15a11a9

… to register adapter for Object or JsonElement

google#2436 - Updates to User guide & comments to indicate exception …

0218548

…cases and fix for 7 test cases of Parameterized Type

Merge branch 'main' into feature/issue#2436_enhancement

0701f89

Marcono1234 reviewed Aug 26, 2023

View reviewed changes

Sachin Patil and others added 3 commits August 28, 2023 16:24

google#2436 - Fixes as per the review comments.

f96af9b

Merge branch 'google:main' into feature/issue#2436_enhancement

840b0b3

Merge branch 'feature/issue#2436_enhancement' of https://github.com/s…

4c04356

…achinp97/gson into feature/issue#2436_enhancement

sachinp97 closed this Aug 28, 2023

sachinp97 reopened this Aug 28, 2023

Marcono1234 reviewed Aug 29, 2023

View reviewed changes

gson/src/main/java/com/google/gson/GsonBuilder.java Outdated Show resolved Hide resolved

gson/src/main/java/com/google/gson/GsonBuilder.java Outdated Show resolved Hide resolved

gson/src/test/java/com/google/gson/GsonBuilderTest.java Outdated Show resolved Hide resolved

Marcono1234 reviewed Aug 29, 2023

View reviewed changes

google deleted a comment from Crazyfortiff Aug 30, 2023

sachinp97 changed the title ~~Feature/issue#2436 enhancement~~ issue#2436: Throw exception when registering adapter for Object or JsonElement Sep 7, 2023

sachinp97 and others added 5 commits September 12, 2023 17:08

Merge branch 'main' into feature/issue#2436_enhancement

11b97ea

Merge branch 'main' into feature/issue#2436_enhancement

7fca65d

google#2436 - Refactored as per latest review comments + throwing err…

d9f42d8

…or message.

Merge branch 'feature/issue#2436_enhancement' of https://github.com/s…

02605d2

…achinp97/gson into feature/issue#2436_enhancement

google#2436 - added a clarifying comment in a positive test case.

5c0ea74

sachinp97 requested review from Marcono1234 and eamonnmcmanus September 19, 2023 14:50

eamonnmcmanus requested changes Sep 19, 2023

View reviewed changes

gson/src/main/java/com/google/gson/GsonBuilder.java Outdated Show resolved Hide resolved

gson/src/main/java/com/google/gson/GsonBuilder.java Outdated Show resolved Hide resolved

Marcono1234 reviewed Sep 20, 2023

View reviewed changes

gson/src/main/java/com/google/gson/GsonBuilder.java Outdated Show resolved Hide resolved

gson/src/test/java/com/google/gson/GsonBuilderTest.java Outdated Show resolved Hide resolved

gson/src/test/java/com/google/gson/GsonBuilderTest.java Outdated Show resolved Hide resolved

sachinp97 and others added 6 commits September 24, 2023 16:33

google#2436 - formatting and minor changes as per review.

263a918

Merge branch 'main' into feature/issue#2436_enhancement

cc01665

Update gson/src/main/java/com/google/gson/GsonBuilder.java

d853261

Co-authored-by: Marcono1234 <[email protected]>

Update gson/src/test/java/com/google/gson/GsonBuilderTest.java

d9025c8

Co-authored-by: Marcono1234 <[email protected]>

Update gson/src/test/java/com/google/gson/GsonBuilderTest.java

ba40b41

Co-authored-by: Marcono1234 <[email protected]>

Merge branch 'main' into feature/issue#2436_enhancement

a068738

Merge branch 'main' into feature/issue#2436_enhancement

fcf2551

sachinp97 requested a review from eamonnmcmanus October 2, 2023 22:12

eamonnmcmanus approved these changes Oct 2, 2023

View reviewed changes

Marcono1234 approved these changes Oct 3, 2023

View reviewed changes

eamonnmcmanus merged commit 0109f45 into google:main Oct 3, 2023
9 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

issue#2436: Throw exception when registering adapter for Object or JsonElement #2479

issue#2436: Throw exception when registering adapter for Object or JsonElement #2479

sachinp97 commented Aug 25, 2023

Marcono1234 left a comment •

edited

Loading

sachinp97 commented Aug 28, 2023

Marcono1234 left a comment

sachinp97 commented Sep 19, 2023

eamonnmcmanus left a comment

Marcono1234 left a comment

sachinp97 commented Sep 29, 2023

eamonnmcmanus left a comment

sachinp97 commented Oct 2, 2023

Marcono1234 left a comment

issue#2436: Throw exception when registering adapter for Object or JsonElement #2479

issue#2436: Throw exception when registering adapter for Object or JsonElement #2479

Conversation

sachinp97 commented Aug 25, 2023

Purpose

Description

Checklist

Marcono1234 left a comment • edited Loading

Choose a reason for hiding this comment

sachinp97 commented Aug 28, 2023

Marcono1234 left a comment

Choose a reason for hiding this comment

sachinp97 commented Sep 19, 2023

eamonnmcmanus left a comment

Choose a reason for hiding this comment

Marcono1234 left a comment

Choose a reason for hiding this comment

sachinp97 commented Sep 29, 2023

eamonnmcmanus left a comment

Choose a reason for hiding this comment

sachinp97 commented Oct 2, 2023

Marcono1234 left a comment

Choose a reason for hiding this comment

Marcono1234 left a comment •

edited

Loading