Enhancements for Json support. #70

avrecko · 2022-12-09T12:16:49Z

on linux unicode problems without + 10 (JsonReader#readHexChar)
call no-arg constructor if annotated with @before (needed for e.g. field init)
if long value outside javascript safe integer range write it as String
support reading long field as String
support reading boolean field as String
support reading String field as boolean or number

avrecko · 2022-12-09T12:20:55Z

Hello, I've made some enhancement for Json support. I think if you do One-Nio Json <-> One-Nio Json some of this is not needed. But if there are 3rd party servers/libraries in between this is needed.

As mentioned in an issue. I can get a long field as String {"foo":"123" or "foo":123} imho this should just work out-of-the-box. Also needing to call constructor in some rare cases. On Linux noticed that I need to add + 10 for unicode to work.

Overall pretty cool code, I found it very logical and easy to change. Let me know if the changes make sense to you.

incubos · 2023-01-17T15:46:51Z

src/one/nio/serial/Json.java

@@ -44,6 +47,16 @@ public static void appendChars(StringBuilder builder, char[] obj) {
        builder.append(obj, from, obj.length - from).append('"');
    }

+    public static void appendLong(StringBuilder builder, long obj) {
+        if (obj < JS_NUMBER_MIN_SAFE_INTEGER | obj > JS_NUMBER_MAX_SAFE_INTEGER) {


|| instead of | would follow the canonical way.
obj < JS_NUMBER_MIN_SAFE_INTEGER || JS_NUMBER_MAX_SAFE_INTEGER < obj might be slightly more readable.

What are your thoughts on | being potentially slightly faster than || as it allows for parallel evaluation?

I can write it like that, no issues.

I would prefer the idiomatic || way until the performance gain is supported by JMH microbenchmarks. Let JIT do its job :)

src/one/nio/serial/JsonReader.java

incubos · 2023-01-17T16:06:25Z

src/one/nio/serial/gen/DelegateGenerator.java

-import one.nio.serial.NotSerial;
-import one.nio.serial.Repository;
-import one.nio.serial.SerializeWith;
+import one.nio.serial.*;


Wildcard imports are not welcome.

incubos · 2023-01-17T16:06:32Z

src/one/nio/serial/gen/DelegateGenerator.java

-import java.lang.reflect.Method;
-import java.lang.reflect.Modifier;
-import java.lang.reflect.ParameterizedType;
+import java.lang.reflect.*;


Wildcard imports are not welcome.

test/one/nio/serial/JsonReaderTest.java

incubos · 2023-01-17T16:32:02Z

test/one/nio/serial/JsonTest.java

+        public long longField;
+    }
+
+    public static class BeforeTestWith implements Serializable {


BeforeTestWithAnnotation?

avrecko · 2023-01-17T17:45:05Z

@incubos thanks for the feedback, after making the pull request I made some further changes. I'll update this pull request with my latest changes just unsure when.

Pretty impressed with the performance and the low level control.

* on linux unicode problems without + 10 (JsonReader#readHexChar) * call no-arg constructor if annotated with @before (needed for e.g. field init) * if long value outside javascript safe integer range write it as text * support assigning parsable json text for java number and boolean fields * support parsing json number and json boolean for java String field

avrecko · 2023-02-19T19:43:58Z

@incubos Finally got around to it. Committed the changes. I've also addressed inheritance with @Before annotation.

avrecko · 2023-03-12T19:38:53Z

@incubos I've added some more features.

Date parsing. Can parse subset of rfc2616 and iso1806
Don't need to implement Serializable for just json serialization. (fwiw: jackson, gson, etc, don't require it)

Maybe you can do a review?

atimofeyev · 2023-03-27T19:12:33Z

src/one/nio/util/DateParser.java

+        return ZonedDateTime.of(year, month, day, hour, minute, second, nanoseconds, ZoneOffset.UTC).toInstant().toEpochMilli();
+    }
+
+    private static long parseRfc2616(String input) {


Rfc2616 is HTTP RFC, did you mean RFC_1123?
ms be we can just call ZonedDateTime.parse("", RFC_1123_DATE_TIME)?

I am referring to 3.3.1 They specify 3 variants:

Sun, 06 Nov 1994 08:49:37 GMT ; RFC 822, updated by RFC 1123
Sunday, 06-Nov-94 08:49:37 GMT ; RFC 850, obsoleted by RFC 1036
Sun Nov 6 08:49:37 1994 ; ANSI C's asctime() format

Only supporting the 1st one.

atimofeyev · 2023-03-28T06:40:41Z

test/one/nio/util/DateParserTest.java

+        Assert.assertEquals(778841377000L, DateParser.parse("Tue, 06 Sep 1994 08:49:37 GMT"));
+        Assert.assertEquals(781433377000L, DateParser.parse("Thu, 06 Oct 1994 08:49:37 GMT"));
+        Assert.assertEquals(784111777000L, DateParser.parse("Sun, 06 Nov 1994 08:49:37 GMT"));
+        Assert.assertEquals(786703777000L, DateParser.parse("Sun, 06 Dec 1994 08:49:37 GMT"));


this date is incorrect, java parser RFC_1123_DATE_TIME failed on it.
It should be "Tue, 06 Dec 1994 08:49:37 GMT"

I don't verify if day of week is correct.

Mon, 06 Dec 1994 08:49:37 GMT is the same as Tue, 06 Dec 1994 08:49:37 GMT, same as Wed, 06 Dec 1994 08:49:37 GMT, etc. for the parser. It just looks for month and the day of the month while ignoring the day of week.

Day of week validation can be added. Let me know what you think.

atimofeyev · 2023-03-28T06:55:43Z

src/one/nio/util/DateParser.java

+        return parseIso1806(input);
+    }
+
+    private static long parseIso1806(String input) {


did you mean ISO-8601?
ZonedDateTime.parse("", ISO_ZONED_DATE_TIME)

could you please tell about use cases where we need support all of it ISO-8601 variants?
ISO_ZONED_DATE_TIME do not support all your variants

Yes, made a typo with 1806 instead of 8601.

My use case is to parse an output from a C++ program. The C++ program encodes microseconds as part of the ISO8601 style date string. Example date string put in json by the C++ program. "2022-08-18T12:01:27.967875+0200".

atimofeyev · 2023-03-28T08:26:17Z

src/one/nio/serial/Json.java

@@ -70,6 +75,18 @@ public static void appendObject(StringBuilder builder, Object obj) throws IOExce
        }
    }

+    public static void appendLong(StringBuilder builder, long value) {


I believe writing long as string should be configurable (as in JACKSON/GSON)

Fine with me.

atimofeyev · 2023-03-28T08:28:38Z

src/one/nio/serial/Before.java

+ */
+@Retention(RetentionPolicy.RUNTIME)
+@Target({ElementType.CONSTRUCTOR})
+public @interface Before {


Is it not enough @default annotation on fields?

Hm, I'll get back to you on this one.

atimofeyev · 2023-03-28T08:32:49Z

src/one/nio/serial/GeneratedSerializer.java

@@ -39,12 +39,15 @@ public class GeneratedSerializer extends Serializer {
    static final AtomicInteger renamedFields = new AtomicInteger();
    static final AtomicInteger unsupportedFields = new AtomicInteger();

+    private final boolean jsonOnlySerialization;


What do you think about creating separate JsonSerializer with own configuration ?
It seems that json serialization can be extended in the fututre and it whould be to heavy for java serializer

Great idea. I'll see what I can come up with.

incubos · 2023-03-28T15:00:40Z

@avrecko ResponseTest.testResponseJsonNotSerializable() doesn't pass anymore. Could you please look at it?

incubos

There are some comments and a compatibility issue we need to eliminate somehow. But the unit tests are awesome!

src/one/nio/serial/Json.java

incubos · 2023-03-28T16:05:56Z

src/one/nio/serial/Repository.java

                    }
-                } else {
-                    serializer = new InvalidSerializer(cls);


Replacement of InvalidSerializer by GeneratedSerializer (JSON-only) looks like a serious behavior change, because GeneratedSerializer implements Serializer methods, but InvalidSerializer throws exceptions. So a user of the upstream version would get an exception if she forgot to implement Serializable, but in this version she would not.

Yeah, I think as suggested by @atimofeyev maybe it makes sense to make a separete JsonSerializer with a bit different behavior. Such as not needing to implement Serializable.

incubos · 2023-03-28T16:33:34Z

src/one/nio/serial/gen/DelegateGenerator.java

-        generateRead(cv, cls, fds, defaultFields, className);
-        generateSkip(cv, fds);
+
+        if (!jsonOnly) {


if with a negated condition and both branches present doesn't look idiomatic, don't you think? Can we switch the branches and eliminate negation.

src/one/nio/serial/gen/DelegateGenerator.java

incubos · 2023-03-28T16:43:19Z

src/one/nio/serial/gen/DelegateGenerator.java

@@ -433,6 +472,19 @@ private static void generateFromJson(ClassVisitor cv, Class cls, FieldDescriptor
        // Create instance
        mv.visitTypeInsn(NEW, Type.getInternalName(cls));

+        // support for calling constructor annotated with @Before
+        for (Class searchClass = cls; searchClass != null; searchClass = searchClass.getSuperclass()) {


The cycle might look tricky to a seasoned reader (I've spent a minute or two to parse the meaning), but I have no suggestions how to simplify that, unfortunately.

Yeah, I am not a fan of constructor call either. Will go trough a few use cases and see if @Default would work.

incubos · 2023-03-28T16:49:23Z

test/one/nio/serial/gen/DelegateGeneratorTest.java

+    }
+
+    @Test
+    public void testBeforeAnnotationSupport() throws IOException, ClassNotFoundException {


It is up to you, but it is safe and might be more concise to use throws Exception in unit tests.

Co-authored-by: Vadim Tsesko <incubos@users.noreply.github.com>

avrecko · 2023-03-28T17:38:45Z

Thank you for doing the review.

I've also added support for not serializing default values. I have not yet synced the changes with this branch.

It does look like it makes a lot of sense to split the Json code in a separete JsonSerializer that is also configurable a little bit.

I'll update the code when I can and we can do the review cycle again.

incubos requested changes Jan 17, 2023

View reviewed changes

incubos self-assigned this Jan 17, 2023

avrecko force-pushed the json_updates branch from 65ea62c to bf06b4a Compare February 19, 2023 17:51

avrecko force-pushed the json_updates branch from bf06b4a to 0ef9a2f Compare February 19, 2023 19:39

Added support for parsing rf2616 and iso1806 date formats in json.

3f17042

avrecko force-pushed the json_updates branch from 6c3a2a5 to 3f17042 Compare March 11, 2023 19:48

Don't need to implement Serializable for json only serialization.

bea5094

avrecko force-pushed the json_updates branch from 5be53f3 to bea5094 Compare March 12, 2023 11:01

incubos requested a review from atimofeyev March 24, 2023 10:58

atimofeyev requested changes Mar 28, 2023

View reviewed changes

incubos requested changes Mar 28, 2023

View reviewed changes

avrecko and others added 2 commits March 28, 2023 19:26

Refined if statement.

6d16a40

Co-authored-by: Vadim Tsesko <incubos@users.noreply.github.com>

Whitespace.

9fb92c3

Co-authored-by: Vadim Tsesko <incubos@users.noreply.github.com>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Enhancements for Json support. #70

Enhancements for Json support. #70

avrecko commented Dec 9, 2022 •

edited by incubos

avrecko commented Dec 9, 2022

incubos Jan 17, 2023

avrecko Jan 17, 2023

incubos Mar 28, 2023

incubos Jan 17, 2023

incubos Jan 17, 2023

incubos Jan 17, 2023

avrecko commented Jan 17, 2023

avrecko commented Feb 19, 2023

avrecko commented Mar 12, 2023 •

edited

atimofeyev Mar 27, 2023

avrecko Mar 28, 2023

atimofeyev Mar 28, 2023

avrecko Mar 28, 2023

atimofeyev Mar 28, 2023

avrecko Mar 28, 2023

atimofeyev Mar 28, 2023

avrecko Mar 28, 2023

atimofeyev Mar 28, 2023

avrecko Mar 28, 2023

atimofeyev Mar 28, 2023

avrecko Mar 28, 2023

incubos commented Mar 28, 2023

incubos left a comment

incubos Mar 28, 2023

avrecko Mar 28, 2023

incubos Mar 28, 2023

incubos Mar 28, 2023

avrecko Mar 28, 2023

incubos Mar 28, 2023

avrecko commented Mar 28, 2023

Enhancements for Json support. #70

Are you sure you want to change the base?

Enhancements for Json support. #70

Conversation

avrecko commented Dec 9, 2022 • edited by incubos

avrecko commented Dec 9, 2022

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

avrecko commented Jan 17, 2023

avrecko commented Feb 19, 2023

avrecko commented Mar 12, 2023 • edited

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

incubos commented Mar 28, 2023

incubos 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

avrecko commented Mar 28, 2023

avrecko commented Dec 9, 2022 •

edited by incubos

avrecko commented Mar 12, 2023 •

edited