(Javalin) Avaje JsonB support #99

SentryMan · 2022-10-18T21:04:53Z

Adds JsonB javalin generation
moves a bunch of jsonb stuff into core module
Test upgraded to Javalin 5

http-generator-core/src/main/java/io/avaje/http/generator/core/ElementReader.java

rbygrave · 2022-10-18T21:50:16Z

Nice. As a general comment, when PRs/diffs get big we increase the desire to have "style changes" in separate PRs from "functional changes" - it makes it easier to see the actual "functional changes" that are occurring. I also see that our IDEs are doing different formatting so the diffs also include format only changes (which also makes it harder to see the actual functional change in the review).

Anyway, great work. Let me know when you are finished with the PR but yes generally we desire "style changes" in a separate PR from "functional changes" just to make it easier to review the diff.

SentryMan · 2022-10-18T22:04:53Z

Nice. As a general comment, when PRs/diffs get big we increase the desire to have "style changes" in separate PRs from "functional changes" - it makes it easier to see the actual "functional changes" that are occurring. I also see that our IDEs are doing different formatting so the diffs also include format only changes (which also makes it harder to see the actual functional change in the review).

Anyway, great work. Let me know when you are finished with the PR but yes generally we desire "style changes" in a separate PR from "functional changes" just to make it easier to review the diff.

Perhaps we could use the Google Java format and Roll with that? Something like the https://github.com/spotify/fmt-maven-plugin?

SentryMan · 2022-10-19T01:21:26Z

Yeah, looks like the issue with this is that JsonB is incorrectly parsing the fields with annotations, I made some changes to Jsonb and it works on my local.

rbygrave · 2022-10-19T20:27:24Z

This PR is getting way too big - its currently +3,043 −229

rbygrave · 2022-10-19T20:29:42Z

Google Java format

This project as got a .editorconfig ... but the issue is that it hasn't got an entry for line width. In the meantime we need to check the diff before commit to not include style changes.

SentryMan · 2022-10-19T20:37:42Z

Most of it is coming from the new test module that caught the bug. Shall I remove?

rbygrave

I'm seeing a long of "style change only" changes ... why don't these go in a separate PR so that we can see the functional changes easier

http-generator-core/src/main/java/io/avaje/http/generator/core/ElementReader.java

rbygrave · 2022-10-20T08:25:40Z

...-generator-javalin/src/main/java/io/avaje/http/generator/javalin/ControllerMethodWriter.java

      if (i > 0) {
        writer.append(", ");
      }
      params.get(i).buildParamName(writer);
    }
-    writer.append(")");
+
+    writer.append(");").eol();


note to check this

rbygrave · 2022-10-20T08:28:14Z

http-generator-javalin/src/main/java/io/avaje/http/generator/javalin/ControllerWriter.java

@@ -46,10 +63,15 @@ private void writeForMethod(MethodReader method) {
  private void writeClassStart() {
    writer.append(AT_GENERATED).eol();
    writer.append("@Component").eol();
-    writer.append("public class ").append(shortName).append("$Route implements WebRoutes {").eol().eol();
+    writer


rbygrave · 2022-10-20T08:29:52Z

http-generator-javalin/src/main/java/io/avaje/http/generator/javalin/JavalinAdapter.java

@@ -58,7 +68,8 @@ public void writeReadParameter(Append writer, ParamType paramType, String paramN
  }

  @Override
-  public void writeReadParameter(Append writer, ParamType paramType, String paramName, String paramDefault) {
+  public void writeReadParameter(


rbygrave · 2022-10-20T08:30:17Z

http-generator-javalin/src/main/java/io/avaje/http/generator/javalin/JavalinProcessor.java

  }

  @Override
-  public void writeControllerAdapter(ProcessingContext ctx, ControllerReader reader) throws IOException {
-    new ControllerWriter(reader, ctx).write();
+  public void writeControllerAdapter(ProcessingContext ctx, ControllerReader reader)


rbygrave · 2022-10-20T08:31:38Z

http-generator-nima/src/main/java/io/avaje/http/generator/helidon/nima/ControllerWriter.java


-  ControllerWriter(ControllerReader reader, ProcessingContext ctx, boolean jsonB) throws IOException {
+  ControllerWriter(ControllerReader reader, ProcessingContext ctx, boolean jsonB)


rbygrave · 2022-10-20T08:32:46Z

http-generator-nima/src/main/java/io/avaje/http/generator/helidon/nima/ControllerWriter.java

-    for (UType type : jsonTypes.values()) {
-      writer.append("  private final JsonType<%s> %sJsonType;", primitiveWrap(type.full()), type.shortName()).eol();
+    for (final UType type : jsonTypes.values()) {
+      writer


rbygrave · 2022-10-20T08:33:01Z

http-generator-nima/src/main/java/io/avaje/http/generator/helidon/nima/ControllerWriter.java

-        writer.append("    this.%sJsonType = jsonB.type(", type.shortName());
-        writeJsonbType(type);
+      for (final UType type : jsonTypes.values()) {
+        JsonBUtil.writeJsonbType(type, writer);


It's the little things in life that mean the most

rbygrave · 2022-10-20T08:34:10Z

http-generator-nima/src/main/java/io/avaje/http/generator/helidon/nima/NimaProcessor.java

@@ -30,7 +30,8 @@ protected PlatformAdapter providePlatformAdapter() {
  }

  @Override
-  public void writeControllerAdapter(ProcessingContext ctx, ControllerReader reader) throws IOException {
+  public void writeControllerAdapter(ProcessingContext ctx, ControllerReader reader)
+      throws IOException {


rbygrave · 2022-10-20T08:36:11Z

tests/test-javalin-jsonb/LICENSE

@@ -0,0 +1,201 @@
+                                 Apache License


oh yeah, I don't think we want this here. We have a LICENSE file at the top of the project and that is expected to cover all content in the project. IMO it's a negative to add extra LICENSE files in sub-directories.

rbygrave · 2022-10-20T08:36:32Z

tests/test-javalin-jsonb/pom.xml

@@ -0,0 +1,156 @@
+<?xml version="1.0" encoding="UTF-8"?>


SentryMan added 3 commits October 18, 2022 14:22

start

5cd1fc5

create JsonB Util

ebdff93

support

d64812f

SentryMan changed the title ~~Javalin Avaje JsonB support~~ (Javalin) Avaje JsonB support Oct 18, 2022

rbygrave reviewed Oct 18, 2022

View reviewed changes

http-generator-core/src/main/java/io/avaje/http/generator/core/ElementReader.java Outdated Show resolved Hide resolved

SentryMan added 2 commits October 18, 2022 17:31

this

a370f37

unstatic jsontype and primitive map

bac1d65

more this

f76600f

SentryMan added 4 commits October 18, 2022 18:20

Merge remote-tracking branch 'upstream/master' into javalinJsonB

4e69924

javalin jsonB test

e5f729e

something is up with JsonB

22e9491

tests finally pass

40c9da2

SentryMan added 2 commits October 19, 2022 00:42

last this

854c2e1

Update README.md

d40a180

rbygrave requested changes Oct 19, 2022

View reviewed changes

SentryMan added 7 commits October 19, 2022 21:10

release 11

a10dd13

remove style

b052639

Update ElementReader.java

20267f4

Update ElementReader.java

64e9322

Update ElementReader.java

7329ca4

Update JavalinProcessorTest.java

cd21c92

Update JavalinProcessorTest.java

85d6f75

SentryMan requested a review from rbygrave October 20, 2022 01:41

rbygrave approved these changes Oct 20, 2022

View reviewed changes

rbygrave assigned SentryMan Oct 20, 2022

rbygrave added this to the 1.19 milestone Oct 20, 2022

rbygrave merged commit 9626702 into avaje:master Oct 20, 2022

SentryMan deleted the javalinJsonB branch October 20, 2022 14:24

SentryMan mentioned this pull request Oct 31, 2022

Add Javalin Jsonb Support #108

Closed


		ControllerWriter(ControllerReader reader, ProcessingContext ctx, boolean jsonB) throws IOException {
		ControllerWriter(ControllerReader reader, ProcessingContext ctx, boolean jsonB)

(Javalin) Avaje JsonB support #99

(Javalin) Avaje JsonB support #99

Uh oh!

Conversation

SentryMan commented Oct 18, 2022

Uh oh!

Uh oh!

rbygrave commented Oct 18, 2022

Uh oh!

SentryMan commented Oct 18, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

SentryMan commented Oct 19, 2022

Uh oh!

rbygrave commented Oct 19, 2022

Uh oh!

rbygrave commented Oct 19, 2022

Uh oh!

SentryMan commented Oct 19, 2022

Uh oh!

rbygrave left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

SentryMan commented Oct 18, 2022 •

edited

Loading