Avoid invalidating Gson type adapter cache when creating Mongo Repository #1406

jmoghisi · 2022-09-29T21:03:45Z

Avoid invalidating Gson type adapter cache when creating Mongo Repository.

We only register the delegatingTypeAdapterFactory if Gson does not have TypeAdapters registered for all Bson related types

jmoghisi · 2022-10-04T07:30:08Z

mongo/src/org/immutables/mongo/bson4gson/GsonCodecs.java

+   * @return {@code gson} configured with delegating type adapter factory.
+   */
+  public static Gson newGsonWithDelegatingTypeAdapterFactory(Gson gson) {
+    if (hasBsonTypeAdapters(gson)) {


@asereda-gs not too happy with this approach but could not identify a better way to detect if these type adapters were already registered. an alternative I considered was to add another builder method to RepositorySetup that allows providing a preconfigured Gson instance. please let me know your thoughts.

elucash · 2022-10-04T08:05:47Z

recovering from Ian storm, will get back in a couple of days, sorry (if interested my zip code is 33947)

talios · 2022-10-04T09:05:06Z

On 4/10/2022 at 9:05:58 PM, Eugene Lukash ***@***.***> wrote:

recovering from Ian storm, will get back in a couple of days, sorry

Off topic - but glad to hear your ok, and hope you weren’t too displaced/affected - looks beyond nasty! Mark

…

-- "Great artists are extremely selfish and arrogant things" — Steven Wilson, Porcupine Tree

elucash · 2022-10-04T09:15:40Z

Thank you Mark @talios for support! We were lucky! No comments

elucash · 2022-10-04T09:18:06Z

only minor damage, happy to survive this shit ))

elucash · 2022-10-11T23:53:55Z

@jmoghisi I've reviewed the PR, and I think I've got the idea. The mongo module is in maintenance mode mostly, newer stuff went into criteria module, including MongoDB backend implementation. I'm ok with these changes overall, but want to minimize the surface of changes. I would propose to remove the changes (and cleanup other corresponding methods) in RepositorySetup.Builder.gson( and instead create a new method RepositorySetup.Builder.preconfiguredGson( where we will take Gson instance as is, essentially what if (hasBsonTypeAdapters(gson)) {... branch is doing. In light of this, it's even possible to get away without this new method, because we already can pass Builder codecRegistry(CodecRegistry registry) which is just 2 calls away from preconfiguredGson via CodecRegistries.fromRegistries(GsonCodecs.codecRegistryFromGson(.

So in the dry down we could have just some new codecs registered, some changes to util, constants, and tests. Plus, if you really wish – add RepositorySetup.Builder.preconfiguredGson( or leave it out in favor of calling RepositorySetup.Builder.codecRegistry and construct gson/codecs manually.

Minor stuff - if possible change line continuation indent to 4 spaces instead of 8 (even if we had it accidental 8 previously). The regular indent is 2 which is correct. Also make ); trail on the previous line, without line break

  smth = wrappingCall(new Something.Something()
      .lineContinuation()  // <-- +4 continuation indent
      .anotherLine());  // <-- no newline before closing );

asereda-gs · 2022-10-12T02:47:23Z

hi @jmoghisi, thanks for this PR.

I tend to agree with Eugene. Maybe modify RepositorySetup to provide "raw" Gson or have a boolean flag to indicate not to enhance existing gson instance.

Also we you can't leverage Builder.codecRegistry() for your usecase ?

jmoghisi · 2022-10-12T05:44:29Z

Thanks you both for the feedback. I've removed the changes to Builder#gson.

Builder#codecRegistry should be sufficient but I just needed to made GsonNamingStrategy public.

asereda-gs · 2022-10-13T02:21:13Z

mongo/src/org/immutables/mongo/bson4gson/GsonCodecs.java

+   * @param gson preconfigured instance
+   * @return {@code gson} configured with delegating type adapter factory.
+   */
+  public static Gson newGsonWithDelegatingTypeAdapterFactory(Gson gson) {


Maybe call it newGsonWithMongoSupport , withBsonSupport(Gson) , addBsonSupport(Gson) or includeBson etc.

WithDelegatingTypeAdapterFactory doesn't fully convey the meaning of this method.

right, it's more important what the purpose of the action is (adds bson support) than in which way it's technically achieved (by delegating type adapter factory)

addressed by renaming it to newGsonWithBsonSupport

asereda-gs · 2022-10-13T02:33:15Z

mongo/src/org/immutables/mongo/repository/RepositorySetup.java

@@ -128,7 +127,7 @@ public String translateName(Member member) {
    class GsonNamingStrategy implements FieldNamingStrategy {


I would move this class one level up it is already inside RepositorySetup.FieldNamingStrategy.

On the other hand, FieldNamingStrategy is already Single Abstract Method (SMA) interface so having a lambda on the top of Gson instance should be trivial to write for clients of this API.

Up to you ...

I've been thinking about it for a short while. Seems that we better have something like GsonNamingStrategy public (or private with public factory method namingStrategyFrom(Gson)) because it's kinda documents that it's possible and should be used sometimes. It is easy to implement with a lambda, but people might have no clue that they ought to do that and will do smth hardcoded.

Perhaps GsonNamingStrategy warrants its own file and be public or, at least, gsonNamingStrategy factory method (also public).

Not 100% sure if it should be inside bson4gson package / GsonCodes class.

Maybe, but given that we think Criteria is the newer implementation and to minimize impact of changes (as it is the goal of some changes asked for this PR) let it be just public GsonNamingStrategy, just can be moved out from interface and be directly nested in RepositorySetup as RepositorySetup.GsonNamingStrategy. I think this will be minimal and cute enough.
P.S. I'll answer on other comments tomorrow morning.

made it public visibility and moved to RepositorySetup

asereda-gs

Overall looks fine to me.

Just small comments before merging.

asereda-gs · 2022-10-17T03:32:02Z

mongo/test/org/immutables/mongo/bson4gson/GsonCodecsTest.java

@@ -22,7 +25,7 @@ public void reflectiveTypeAdapter() {
  }

  @Test
-  public void dateCodec() throws IOException {
+  public void typeAdapterFromCodec() throws IOException {


I think keeping the old name (dateCodec) makes more sense

I went with typeAdapterFromDateCodec

asereda-gs · 2022-10-17T03:32:34Z

mongo/test/org/immutables/mongo/bson4gson/GsonCodecsTest.java

@@ -36,4 +39,16 @@ public void dateCodec() throws IOException {
    check(doc.get("$date").getBsonType()).is(BsonType.DATE_TIME);
    check(doc.get("$date").asDateTime().getValue()).is(date.getTime());
  }
+
+  @Test
+  public void newGsonWithDelegatingTypeAdapterFactory() {


Maybe call it gsonWithBsonSupport

elucash · 2022-10-18T02:06:55Z

Thank you! I think this is good and useful! I'm merging this. The release will be when I'll have my broadband internet restored, should be soon, but no ETA

elucash · 2022-10-18T02:09:24Z

I'll also squash this (I'm forgetting sometimes to squash other PRs, sorry), so you'll see one commit in the master.

jmoghisi · 2022-10-18T04:25:07Z

Thanks Eugene, appreciate all the feedback as well.

jmoghisi force-pushed the gson_reuse branch from e096e02 to 4a5e27c Compare September 29, 2022 21:45

jmoghisi commented Oct 4, 2022

View reviewed changes

jmoghisi force-pushed the gson_reuse branch from 4a5e27c to 3c17b2c Compare October 12, 2022 05:33

jmoghisi force-pushed the gson_reuse branch from 3c17b2c to 0c329fa Compare October 12, 2022 05:48

asereda-gs reviewed Oct 13, 2022

View reviewed changes

jmoghisi force-pushed the gson_reuse branch from 0c329fa to 48e77c4 Compare October 16, 2022 11:25

asereda-gs approved these changes Oct 17, 2022

View reviewed changes

jmoghisi added 3 commits October 17, 2022 23:36

add Gson Repo test

566f9b3

add test coverage for GsonCodecs

a06561c

make Gson FieldNamingStrategy public

088b3cd

jmoghisi force-pushed the gson_reuse branch from 48e77c4 to 088b3cd Compare October 17, 2022 22:37

elucash merged commit c3344b5 into immutables:master Oct 18, 2022

jmoghisi deleted the gson_reuse branch October 18, 2022 04:25

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Avoid invalidating Gson type adapter cache when creating Mongo Repository #1406

Avoid invalidating Gson type adapter cache when creating Mongo Repository #1406

jmoghisi commented Sep 29, 2022

jmoghisi Oct 4, 2022

elucash commented Oct 4, 2022 •

edited

talios commented Oct 4, 2022 via email

elucash commented Oct 4, 2022

elucash commented Oct 4, 2022

elucash commented Oct 11, 2022 •

edited

asereda-gs commented Oct 12, 2022

jmoghisi commented Oct 12, 2022

asereda-gs Oct 13, 2022 •

edited

elucash Oct 13, 2022 •

edited

jmoghisi Oct 16, 2022

asereda-gs Oct 13, 2022 •

edited

elucash Oct 13, 2022 •

edited

asereda-gs Oct 13, 2022

elucash Oct 13, 2022 •

edited

jmoghisi Oct 16, 2022

asereda-gs left a comment

asereda-gs Oct 17, 2022

jmoghisi Oct 17, 2022

asereda-gs Oct 17, 2022

jmoghisi Oct 17, 2022

elucash commented Oct 18, 2022

elucash commented Oct 18, 2022

jmoghisi commented Oct 18, 2022

		@@ -128,7 +127,7 @@ public String translateName(Member member) {
		class GsonNamingStrategy implements FieldNamingStrategy {

Avoid invalidating Gson type adapter cache when creating Mongo Repository #1406

Avoid invalidating Gson type adapter cache when creating Mongo Repository #1406

Conversation

jmoghisi commented Sep 29, 2022

Choose a reason for hiding this comment

elucash commented Oct 4, 2022 • edited

talios commented Oct 4, 2022 via email

elucash commented Oct 4, 2022

elucash commented Oct 4, 2022

elucash commented Oct 11, 2022 • edited

asereda-gs commented Oct 12, 2022

jmoghisi commented Oct 12, 2022

asereda-gs Oct 13, 2022 • edited

Choose a reason for hiding this comment

elucash Oct 13, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

asereda-gs Oct 13, 2022 • edited

Choose a reason for hiding this comment

elucash Oct 13, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

elucash Oct 13, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

asereda-gs 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

elucash commented Oct 18, 2022

elucash commented Oct 18, 2022

jmoghisi commented Oct 18, 2022

elucash commented Oct 4, 2022 •

edited

elucash commented Oct 11, 2022 •

edited

asereda-gs Oct 13, 2022 •

edited

elucash Oct 13, 2022 •

edited

asereda-gs Oct 13, 2022 •

edited

elucash Oct 13, 2022 •

edited

elucash Oct 13, 2022 •

edited