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

Pass on method annotations to moshi when requesting adapters #83

Closed
ZacSweers opened this issue Feb 27, 2017 · 8 comments · Fixed by #153
Closed

Pass on method annotations to moshi when requesting adapters #83

ZacSweers opened this issue Feb 27, 2017 · 8 comments · Fixed by #153
Milestone

Comments

@ZacSweers
Copy link
Collaborator

ZacSweers commented Feb 27, 2017

Not sure how this would work, but it would alleviate a ton of feature requests for features that this library probably shouldn't have to worry about by deferring to the moshi instance. Similar to how retrofit passes on annotations as well. This should make something like @serj-lotutovici's lazy adapters work with av models

@rharter thoughts? Might not be trivial, but I think it'd be a worthwhile endeavor

@rharter
Copy link
Owner

rharter commented Feb 27, 2017

Sounds like a reasonable idea. Does Moshi have an API for requesting adapters by type and annotation?

@ZacSweers
Copy link
Collaborator Author

So there's two ways I'd see this working:

  • Pull annotations off the methods at runtime. Requires having a reference to the actual Method at runtime. Delves into reflection territory and proguard issues
  • Copy annotations directly into the generated code. I don't actually know what would go into this, but there is some precedent in AutoValue itself for doing this.

I'm going to explore the second option a bit as I think that would be the ideal case.

@ZacSweers
Copy link
Collaborator Author

Hmm, looks like there's no way around this without doing some reflection unless we want to create a ton of boilerplate anonymous Annotation classes to simulate instances. AutoValue is cute by basically recording a stringified version of the annotations and writing them in directly, but that unfortunately doesn't help us.

Some ideas

  • Reflectively get the method during adapter construction (MyClass.getMethod("a").getAnnotations()). Do it lazily and all reflection
  • Be smarter by checking during compile time that a given annotation on a method with the @JsonQualifier annotation exists (e.g. don't inline unnecessarily, which I think would be most)
  • Avoid obfuscation issues (but not reflection entirely) by annotating all with @Json(name = "whatevs"), then look them up in one loop through declaredMethods() and match them?
  • Find some other way of making methods look-up-able in a way that doesn't suck

@ZacSweers
Copy link
Collaborator Author

ZacSweers commented Feb 27, 2017

Alternative idea - borrow AutoValue's trick, but inline the generated annotations onto generated getters or fields for the adapters. Then we at least have a deterministic model for pulling their information.

Getters (uglier, but guaranteed to work with annotations since they must be able to target methods)

public static final class MoshiJsonAdapter extends JsonAdapter<WithJsonQualifierAnnotationsObject> {
  private final JsonAdapter<String> aAdapter;
  private final JsonAdapter<List<String>> bAdapter;

  public MoshiJsonAdapter(Moshi moshi) {
    this.aAdapter = moshi.adapter(String.class,
        new LinkedHashSet<Annotation>(getClass().getDeclaredMethod("aAnnotationsHolder")
            .getAnnotations()));
    this.bAdapter = adapter(moshi, "b");
  }

  @ReverseList void aAnnotationsHolder() {

  }
}

Or setters (might be iffy if the target annotation doesn't work with fields for some reason, but no empty placeholder methods!)

public static final class MoshiJsonAdapter extends JsonAdapter<WithJsonQualifierAnnotationsObject> {
  @ReverseList private final JsonAdapter<String> aAdapter;
  private final JsonAdapter<List<String>> bAdapter;

  public MoshiJsonAdapter(Moshi moshi) {
    this.aAdapter = moshi.adapter(String.class,
        new LinkedHashSet<Annotation>(getClass().getDeclaredField("aAnnotationsHolder")
            .getAnnotations()));
    this.bAdapter = adapter(moshi, "b");
  }
}

Both could potentially be smarter by accessing the known index directly (e.g. getFields()[1]), but I'm not sure how safe this is with different compilers/runtimes.

@ZacSweers
Copy link
Collaborator Author

I'm leaning toward this:

  • inline annotations onto fields. Caveat is that your qualifiers must work on fields.
  • intelligently only inline detected annotations that are annotated with @JsonQualifier. If there's nothing, then we don't generate any special annotation handling
  • if we detect the existence of @Keep on the classpath, bolt those on to the fields too to protect them in proguard. Otherwise the only rule I can think of is to protect fields in anything called MoshiJsonAdapter :/

@swankjesse would be curious to hear your thoughts on all this too

@ZacSweers
Copy link
Collaborator Author

ZacSweers commented Feb 27, 2017

WAIT this already appears to exist 😨 #24

SoooOoOoOo I guess the question now becomes - is it worth optimizing further per the above?

(excuse me while I get this egg off my face)

@ZacSweers ZacSweers added this to the 0.5.0 milestone Nov 18, 2018
@ZacSweers
Copy link
Collaborator Author

I think we can do this the way we do in moshi code gen - require field site target support and bolt them on to the adapter property.

We would need to borrow these rules if we can't just rely on moshi's embedded rules directly https://github.com/square/moshi/blob/master/moshi/src/main/resources/META-INF/proguard/moshi.pro#L31-L61

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants