Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 4 commits into from
Aug 22, 2023
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions gson/src/main/java/com/google/gson/stream/JsonWriter.java
Original file line number Diff line number Diff line change
Expand Up @@ -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
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

got it! removed this unecessary comment

if (stackSize == 1 && ( peek() == EMPTY_DOCUMENT || peek() == NONEMPTY_DOCUMENT )) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

my bad, corrected that, thanks for pointing out

throw new IllegalStateException("Please begin an object before this.");
}
deferredName = name;
return this;
}
Expand Down
37 changes: 31 additions & 6 deletions gson/src/test/java/com/google/gson/stream/JsonWriterTest.java
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,8 @@
import java.math.BigDecimal;
import java.math.BigInteger;
import org.junit.Test;
import java.util.Arrays;
import java.util.List;

@SuppressWarnings("resource")
public final class JsonWriterTest {
Expand Down Expand Up @@ -114,13 +116,36 @@ public void testTopLevelValueTypes() throws IOException {
public void testInvalidTopLevelTypes() throws IOException {
StringWriter stringWriter = new StringWriter();
JsonWriter jsonWriter = new JsonWriter(stringWriter);
jsonWriter.name("hello"); // TODO: This should throw, see https://github.com/google/gson/issues/2407
try {
jsonWriter.value("world");
fail();
} 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
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

}

@Test
public void closeAllObjectsAndTryToAddElements() throws IOException {
StringWriter stringWriter = new StringWriter();
JsonWriter jsonWriter = new JsonWriter(stringWriter);
jsonWriter.beginObject();
jsonWriter.name("a").value(20);
jsonWriter.name("age").value(30);

// Start the nested "address" object
jsonWriter.name("address").beginObject();
jsonWriter.name("city").value("New York");
jsonWriter.name("country").value("USA");
jsonWriter.endObject(); // End the nested "address" object
jsonWriter.name("random_prop").value(78);
// Add an array of phone numbers (list of numbers)
List<Integer> phoneNumbers = Arrays.asList(1234567890, 98989, 9909);
jsonWriter.name("phoneNumbers").beginArray();
for (Integer phoneNumber : phoneNumbers) {
jsonWriter.value(phoneNumber);
}
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"));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

jsonWriter.close();

}

@Test
Expand Down