Support custom annotation processing by fwonce · Pull Request #1029 · google/gson

fwonce · 2017-03-02T09:32:16Z

This discussion and issue #269 talks about the same problem I want to tackle with. I commented under issue #269 to state my reason and write a subject in Gson Group to talk about more detailed design thoughts. Generally speaking, I'd like to see a feature that let me use custom annotations in my data object freely without exposing Gson stuff there, and control the behaviors of reading/writing the fields with those annotations by Gson.

googlebot · 2017-03-02T09:32:20Z

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed, please reply here (e.g. I signed it!) and we'll verify. Thanks.

If you've already signed a CLA, it's possible we don't have your GitHub username or you're using a different email address. Check your existing CLA data and verify that your email is set on your git commits.
If you signed the CLA as a corporation, please let us know the company's name.

fwonce · 2017-03-02T10:28:47Z

I signed it!

googlebot · 2017-03-02T10:28:49Z

CLAs look good, thanks!

inder123 · 2017-03-10T02:31:26Z

Overall, I like where you are going with this.
A few questions:

Why not support annotation on classes as well?
Would be great if we can demonstrate some real use cases. For example, taking JPA or JAXB annotations, and automatically using them as SerializedName.

Thanks

fwonce · 2017-03-10T17:34:44Z

Hi @inder123 Thanks for your inspiring reply.
Your first advise makes sense to me so I add a few lines to do that. And when considering it, I've realized that in some cases a FieldAdapterFactory should be versatile for both kinds of annotations as a best practice (like d722d40).
But can't write tests for real use cases right now because I do this in spare time. I'll complete it asap:)

EDIT: Is it appropriate to add 3rd-pty test scope dependencies like JPA / JAXB stuff to your pom for writing test cases? Or you mean just give some examples in javadoc?

inder123 · 2017-03-10T20:44:16Z

You can add JPA/JAXB as test dependencies. We will factor them out in a separate project as this progresses. Thank you!

fwonce · 2017-03-13T14:01:11Z

Speaking of JPA/JAXB, I think the most wanted feature is using the name specified by @Column (JPA) or @XmlElement (JAXB) as the serialized name. So I wrote a test. Before that I had to add the Gson instance as the first argument to FieldAdapterFactory#create. And then I did something like:

Column column = (Column) annotation;
final columnName = column.name();
return new TypeAdapter<T>() {
  @Override public void write(JsonWriter out, T value) throws IOException {
    // ... nullcheck ...
    out.name(columnName); // >>>> Unfortunately this will raise a IllegalStateException <<<<
    TypeAdapter adapter = gson.getAdapter(value.getClass());
    adapter.write(out, value);
  }
  // ... read ...
}

The 6th line will raise a IllegalStateException because ReflectiveTypeAdapterFactory.Adapter#write has already called out.name() right before it and probably there is no simple way to get around unless a major refactor is taken.

Deprecated: So at this point custom annotation processing should be good at dealing with the value part instead of the name part, for which a custom FieldNamingStrategy is more suitable, I think.

On the other hand, I wrote some meaningful tests regarding JPA indeed, like ignoring @Transient or @Version fields when serialization. But I'm not sure if that meets your expectation. Or you may give more information about what kind of use cases you would like to see?

EDIT (21 Apr 2017):
There IS a way to generate the wanted serialized names - kind of tricky. Updated the JPA example

sot of unconvincing to me. so this could be ditched somehow.

inder123 · 2017-03-14T02:33:39Z

Great progress. @JakeWharton @swankjesse Can you review this as well please?
I think this generic way of binding annotation processing is what we want.

…ined in runtime Gson singleton if this feature not used at all

swankjesse

I’m reluctant to add something this powerful to Gson, particularly in this style.

When we did Moshi we decided qualifier annotations were worth including, but we wired them in everywhere – in Moshi’s equivalent of the TypeAdapterFactory. With this we’re introducing a fourth way to link a fieldto an adapter that can encode or decode it (others are @JsonAdapter, TypeAdapter, and JsonSerializer/JsonDeserializer).

I think you’re better off just using @JsonAdapter instead of pushing more complexity and capability into Gson.

swankjesse · 2017-03-18T19:38:28Z

gson/src/main/java/com/google/gson/FieldAdapterFactory.java

@@ -0,0 +1,48 @@
+/*
+ * Copyright (C) 2008 Google Inc.


Sure. I copied the license lines from the bottom of google/gson's readme page, felt weird at first, but didn't think about it again. Maybe you guys should update it too.

swankjesse · 2017-03-18T19:39:01Z

gson/src/main/java/com/google/gson/FieldAdapterFactory.java

+ * be ignored.
+ *
+ * @author Floyd Wan
+ * @since 2.8.1


unlikely that we’ll release new APIs in a patch release.

swankjesse · 2017-03-18T19:42:14Z

gson/src/main/java/com/google/gson/Gson.java

+   * @return the {@link TypeAdapter} for the given {@code field}
+   * @since 2.8.1
+   */
+  public <T> TypeAdapter<T> getFieldAdapter(Field field) {


I do like this shortcut

swankjesse · 2017-03-18T19:43:37Z

gson/src/main/java/com/google/gson/Gson.java

+      return null;
+    }
+    for (Annotation fieldAnnotation : field.getAnnotations()) {
+      Class<? extends Annotation> annotationType = fieldAnnotation.annotationType();


might be more efficient to loop over the factories and call field.isAnnotationPresent() rather than looping over the field’s annotations. Only because isAnnotationPresent() doesn’t allocate, but getAnnotations() needs to build instances of all of the field’s annotations.

Thanks! I'll pay attention to that every time in the future.

swankjesse · 2017-03-18T19:43:54Z

gson/src/main/java/com/google/gson/Gson.java

+        return fieldAdapterFactory.create(fieldAnnotation, field);
+      }
+    }
+    // If both this field and its declaring class matched, ignore the latter one


precedence rules? ick

Removed the 'misleading' comment, but I think the logic here is natural - firtst look at the field annotation, and then the class annotations...

fwonce · 2017-03-20T06:42:55Z

Hi @swankjesse!
First, thanks for your review. I have updated according to your inline comments but I'm not sure this is necessary, since you said you are reluctant about this whole idea.

fwonce · 2017-04-17T06:17:09Z

Hi @swankjesse , you haven't made any further comment. Does it mean this is not going to be accepted. In fact, your last comment seemed quite ambiguous to me. The "style" in which to archive this goal can be discussed, if only new features can be assessed and accepted, w/o assuming something too powerful is not welcomed by Gson. Did you suggest that Gson is on its process of going into inactive development and the maintenance team is moving onto something new called Moshi (sorry I haven't heard about it before)?

@inder123 @JakeWharton What's your opinion?

swankjesse · 2017-04-17T12:17:38Z

I’m reluctant to add large new features to Gson at this time. Large features need careful design and I’m not interested in spending that time to review them.

It’s a popular library and it is very difficult to anticipate the compatibility consequences of any change.

Note that although the project originated at Google, no Google employees are involved at this time.

fwonce · 2017-04-19T06:05:55Z

Got it. For now my only choice is to maintain my own fork. Nevertheless, I have to say I'm quite confident that this PR is backward compatible and solid enough:) In my company, I have applied this version of Gson to many production applications for months and it works fine. There are several interesting use cases enabled by this. Hope you guys will have a second look in the future.

By the way, does Moshi support such kind of feature?

inder123 · 2017-04-20T01:47:14Z

@swankjesse I think we have been looking for such a powerful feature for quite some time. This may be the fourth way, but for now, it is the only way to make existing annotations work with Gson. I am highly supportive of this, but am fine with looking at an alternate design.

googlebot · 2017-04-21T16:27:42Z

We found a Contributor License Agreement for you (the sender of this pull request), but were unable to find agreements for the commit author(s). If you authored these, maybe you used a different email address in the git commits than was used to sign the CLA (login here to double check)? If these were authored by someone else, then they will need to sign a CLA as well, and confirm that they're okay with these being contributed to Google.

googlebot · 2017-04-21T16:34:17Z

CLAs look good, thanks!

…apter

googlebot · 2017-07-31T07:34:03Z

We found a Contributor License Agreement for you (the sender of this pull request), but were unable to find agreements for the commit author(s). If you authored these, maybe you used a different email address in the git commits than was used to sign the CLA (login here to double check)? If these were authored by someone else, then they will need to sign a CLA as well, and confirm that they're okay with these being contributed to Google.
In order to pass this check, please resolve this problem and have the pull request author add another comment and the bot will run again.

fm4n added 2 commits March 2, 2017 15:59

support custom annotation processing, [to google#269](google#269)

3cdfe99

add test to google#269

64ba11f

fm4n added 2 commits March 6, 2017 09:50

add license lines to new files

200825e

forgot to make it unmodifiable

e9526cc

fm4n added 3 commits March 11, 2017 00:20

first, add a test that will fail for now

c13a968

remove the unnecessary if-condition

d722d40

support ann on class also

5db0d76

add use case demonstrating JPA annotation involvement.

3740377

sot of unconvincing to me. so this could be ditched somehow.

fm4n added 2 commits March 14, 2017 14:34

save the memory footprint a little bit: no field adapter HashMap reta…

1d399ed

…ined in runtime Gson singleton if this feature not used at all

sync with upstream and resolve conflict

3b9724d

swankjesse reviewed Mar 18, 2017

View reviewed changes

optimize based on swankjesse's review

343adf2

fwonce force-pushed the field-adapter branch from 0a6aa78 to 343adf2 Compare April 21, 2017 16:34

fm4n and others added 2 commits April 22, 2017 00:38

demonstrate jpa serialized name use case

7d15f6b

Merge branch 'master' of https://github.com/google/gson into field-ad…

952412f

…apter

google-cla bot added the cla: no label Nov 11, 2021

Conversation

fwonce commented Mar 2, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

googlebot commented Mar 2, 2017

Uh oh!

fwonce commented Mar 2, 2017

Uh oh!

googlebot commented Mar 2, 2017

Uh oh!

inder123 commented Mar 10, 2017

Uh oh!

fwonce commented Mar 10, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

inder123 commented Mar 10, 2017

Uh oh!

fwonce commented Mar 13, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

inder123 commented Mar 14, 2017

Uh oh!

swankjesse left a comment

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!

fwonce commented Mar 20, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

fwonce commented Apr 17, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

swankjesse commented Apr 17, 2017

Uh oh!

fwonce commented Apr 19, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

inder123 commented Apr 20, 2017

Uh oh!

googlebot commented Apr 21, 2017

Uh oh!

googlebot commented Apr 21, 2017

Uh oh!

googlebot commented Jul 31, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

fwonce commented Mar 2, 2017 •

edited

Loading

fwonce commented Mar 10, 2017 •

edited

Loading

fwonce commented Mar 13, 2017 •

edited

Loading

fwonce commented Mar 20, 2017 •

edited

Loading

fwonce commented Apr 17, 2017 •

edited

Loading

fwonce commented Apr 19, 2017 •

edited

Loading