Changes to ensure if no object is opened user won't be able to write property name #2475

shivam-sehgal · 2023-08-21T17:22:00Z

Purpose

Currently, even if the no object is opened or begin object is not called or all open objects are ended using jsonwriter.endObject() Still user is able to add json object property name, which shouldn't be happening
Closes #2407

Description

Currently when the Jsonwriter name method is called no check is there to identify if it's an empty document or all open objects are closed and stack size reaches 1 and the top element is NON_EMPTY_DOC basically the two case

The user calls jsonwrite.name() for the first time in an empty document or when the beginobject method has not been called.
The user calls jsonwrite.name() when all opened objects are closed.
In both of these invalid cases, the stack size should be 1 as per the current implementation 
In the first case, stack will be having one status only which will be EMPTY_DOCUMENT
 In the second case, all other statuses will be popped from the stack and only one status NONEMPTY_DOCUMENT will be present in the stack
So added a check in the name method for these 2 special cases
Also added and changed the unit test as per the requirement

Checklist

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

…op in the object

eamonnmcmanus

Thanks, this looks great! I have some minor comments. I'm also going to try running this against all of Google's internal tests, though I don't expect that will find any problems.

eamonnmcmanus · 2023-08-21T17:31:30Z

gson/src/main/java/com/google/gson/stream/JsonWriter.java

@@ -498,6 +498,10 @@ public JsonWriter name(String name) throws IOException {
    if (stackSize == 0) {
      throw new IllegalStateException("JsonWriter is closed.");
    }
+    // As currently in any valid case name function cannot be called if stack top has these 2 statuses
+    if (stackSize == 1 && ( peek() == EMPTY_DOCUMENT || peek() == NONEMPTY_DOCUMENT )) {


Style nit: there shouldn't be a space after ( or before ).

my bad, corrected that, thanks for pointing out

eamonnmcmanus · 2023-08-21T17:32:44Z

gson/src/test/java/com/google/gson/stream/JsonWriterTest.java

+    jsonWriter.endArray(); // End the array
+    jsonWriter.endObject(); // End the outer object
+    assertThrows(IllegalStateException.class, () -> jsonWriter.name("this_throw_exception_as_all_objects_are_closed"));
+    assertThrows(IllegalStateException.class, () -> jsonWriter.value("this_throw_exception_as_only_one_top_level_entry"));


I'm a little uneasy at the assumption that the previous statement hasn't changed the state of jsonWriter. I think it might be better to move the preceding setup into a private method (that maybe returns a JsonWriter) and then use that method in two different test methods, one for .name and one for .value.

agree with you regarding status change and the possible impact of it, created a private method to return jsonwritter as you recommended, and running both tests with 2 separate objects

eamonnmcmanus · 2023-08-21T17:59:33Z

gson/src/test/java/com/google/gson/stream/JsonWriterTest.java

-    } catch (IllegalStateException expected) {
-      assertThat(expected).hasMessageThat().isEqualTo("Nesting problem.");
+    assertThrows(IllegalStateException.class, () -> jsonWriter.name("hello"));
+    //removed nested exception test on jsonwriter.writevalue as it seems currently valid case per code


I think you are right about removing that test, but now that you have recorded that, can you remove this comment? It will be confusing for future readers.

makes sense, reading this comment will be odd for users as code is already gone, thanks for pointing it out, removed this comment

eamonnmcmanus · 2023-08-21T18:01:09Z

gson/src/main/java/com/google/gson/stream/JsonWriter.java

@@ -498,6 +498,10 @@ public JsonWriter name(String name) throws IOException {
    if (stackSize == 0) {
      throw new IllegalStateException("JsonWriter is closed.");
    }
+    // As currently in any valid case name function cannot be called if stack top has these 2 statuses


I found this comment a bit confusing. I think it isn't really necessary anyway? It doesn't really tell the reader anything that they can't already see from the following statement.

got it! removed this unecessary comment

shivam-sehgal · 2023-08-21T19:21:11Z

Thank you for reviewing the code! I'll look forward to the test results.

eamonnmcmanus

Thanks for doing this! I ran it against all of Google's internal tests and did not see any problems.

shivam-sehgal · 2023-08-22T18:12:48Z

Thanks for doing this! I ran it against all of Google's internal tests and did not see any problems.

Thanks for the approval :)

Marcono1234 · 2023-08-22T20:21:18Z

Thanks @shivam-sehgal for your work on this!

Unfortunately I think my original issue made it look like this only affected the top level (which is addressed by your pull request), but it actually also affects trying to use JsonWriter.name inside a JSON array.

I have created #2476 to (hopefully) fully resolve this. It reverts some of your test changes because handling of multiple top-level values was already covered by multiple separate test methods, and while it is generally good that your method getJsonWriterWithObjects covered multiple value types; I think here more concise test data specific to this issue might be better. The data created by getJsonWriterWithObjects might rather be useful for integration tests / functional tests (I think) but there already a bunch of this in this repository. Sorry for that; I hope it is ok for you (and for @eamonnmcmanus).

…property name (google#2475) * Changes to ensure if no object is opened user won't be able to add prop in the object * review points * spelling correction

Changes to ensure if no object is opened user won't be able to add pr…

bca3acb

…op in the object

shivam-sehgal mentioned this pull request Aug 21, 2023

JsonWriter.name does not throw exception when not inside JSON object #2407

Closed

eamonnmcmanus requested changes Aug 21, 2023

View reviewed changes

shivam-sehgal added 2 commits August 22, 2023 00:39

review points

834cc62

spelling correction

9fb6da5

shivam-sehgal requested a review from eamonnmcmanus August 22, 2023 03:11

Merge branch 'main' into bug/2407

4c0e0fb

eamonnmcmanus approved these changes Aug 22, 2023

View reviewed changes

eamonnmcmanus merged commit 392cc65 into google:main Aug 22, 2023

Marcono1234 mentioned this pull request Aug 22, 2023

Throw exception when calling JsonWriter.name outside JSON object #2476

Merged

9 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Changes to ensure if no object is opened user won't be able to write property name #2475

Changes to ensure if no object is opened user won't be able to write property name #2475

shivam-sehgal commented Aug 21, 2023 •

edited

Loading

eamonnmcmanus left a comment

eamonnmcmanus Aug 21, 2023

shivam-sehgal Aug 21, 2023

eamonnmcmanus Aug 21, 2023

shivam-sehgal Aug 21, 2023

eamonnmcmanus Aug 21, 2023

shivam-sehgal Aug 21, 2023

eamonnmcmanus Aug 21, 2023

shivam-sehgal Aug 21, 2023

shivam-sehgal commented Aug 21, 2023

eamonnmcmanus left a comment

shivam-sehgal commented Aug 22, 2023

Marcono1234 commented Aug 22, 2023

Changes to ensure if no object is opened user won't be able to write property name #2475

Changes to ensure if no object is opened user won't be able to write property name #2475

Conversation

shivam-sehgal commented Aug 21, 2023 • edited Loading

Purpose

Description

Checklist

eamonnmcmanus left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shivam-sehgal commented Aug 21, 2023

eamonnmcmanus left a comment

Choose a reason for hiding this comment

shivam-sehgal commented Aug 22, 2023

Marcono1234 commented Aug 22, 2023

shivam-sehgal commented Aug 21, 2023 •

edited

Loading