mattlogan / auto-value-firebase

AutoValue extension for creating Firebase Realtime Database objects
102 stars 6 forks source link

Generated class should not always be final. #1

Closed f2prateek closed 8 years ago

f2prateek commented 8 years ago

(I haven't dug into how auto value extensions work yet so some stuff might be completely off!)

It looks like the classes generated by this extension are always final. I believe the way extensions works is that each extension generates a class that extends from a class generated by another extension. This means that for multiple extensions, applying the firebase extension last is a requirement.

If the firebase extension is not applied last, a class generated by another extension cannot cannot extend the firebase extension class, and will not compile. Here's an example error:

error: cannot inherit from final $AutoValue_Tracker

I think this specific line https://github.com/mattlogan/auto-value-firebase/blob/master/auto-value-firebase/src/main/java/me/mattlogan/auto/value/firebase/AutoValueFirebaseExtension.java#L71 causes it to always generate a final class.

I think fix would probably be to conditionally apply final, like in the moshi extension https://github.com/rharter/auto-value-moshi/blob/master/src/main/java/com/ryanharter/auto/value/moshi/AutoValueMoshiExtension.java#L157-L161.

Reproduce

Given the AutoValue class:

@AutoValue @FirebaseValue //
public abstract class Foo {
  public abstract String bar();

  public static JsonAdapter<Tracker> jsonAdapter(Moshi moshi) {
    return new AutoValue_Tracker.MoshiJsonAdapter(moshi);
  }

  public static Tracker fromDataSnapshot(DataSnapshot dataSnapshot) {
    AutoValue_Tracker.FirebaseValue tracker =
        dataSnapshot.getValue(AutoValue_Tracker.FirebaseValue.class);
    return new AutoValue_Tracker(tracker);
  }

  public Object toFirebaseValue() {
    return new AutoValue_Tracker.FirebaseValue(this);
  }
}

Applying Firebase Extension last works

build.gradle:

  compile 'com.google.auto.value:auto-value:1.3-rc1'
  apt 'me.mattlogan.auto.value:auto-value-firebase:0.1.1'
  provided 'me.mattlogan.auto.value:auto-value-firebase-annotation:0.1.1'
  apt 'com.ryanharter.auto.value:auto-value-moshi:0.3.3-rc1'

This will work. The generated hierarchy is AutoValue_Tracker (by the firebase extension, so final doesn't matter since it's the last in the chain) extends $AutoValue_Tracker (by the moshi extension) extends $$AutoValue_Tracker (by auto value) extends Tracker.

Applying Firebase Extension first does not work

build.gradle:

  compile 'com.google.auto.value:auto-value:1.3-rc1'
  apt 'com.ryanharter.auto.value:auto-value-moshi:0.3.3-rc1'
  provided 'me.mattlogan.auto.value:auto-value-firebase-annotation:0.1.1'
  apt 'me.mattlogan.auto.value:auto-value-firebase:0.1.1'

Will not work. The generated hierarchy is AutoValue_Tracker (by the moshi extension) extends $AutoValue_Tracker (by the firebase extension which is final) extends $$AutoValue_Tracker (by auto value) extends Tracker.

This will fail with the error error: cannot inherit from final $AutoValue_Tracker since the moshi extension cannot extend the final $AutoValue_Tracker class generated by the firebase extension.

mattlogan commented 8 years ago

Thanks for the detailed report. A few comments here:

First of all, I'm assuming the class name in your example should be Tracker, not Foo.

Making the generated class final at line 71 was a mistake, and I didn't intend for my generated classes to be final. Thanks for catching this.

If we remove that line, the generated class isn't final, and we are able to generate both AutoValue classes. However, in this case, the example still won't compile if we apply the Firebase extension first. This is because the generated Firebase extension class contains the constructor with the FirebaseValue parameter, and this constructor is absent on the Moshi extension class. As a result, return new AutoValue_Tracker(tracker); doesn't compile, because the Moshi extension class only contains the standard AutoValue constructor with a String parameter.

Here's an idea for getting around this:

Instead of adding a constructor in the generated class with a FirebaseValue parameter, we could instead add a method in the FirebaseValue class called toAutoValue(). That implementation would look like this:

  static final class FirebaseValue {
    private String bar;
    @SuppressWarnings("unused")
    FirebaseValue() {
    }
    FirebaseValue(Tracker tracker) {
      this.bar = tracker.bar();
    }
    public String getBar() {
      return bar;
    }
    @Exclude
    public AutoValue_Tracker toAutoValue() {
      return new AutoValue_Tracker(bar);
    }
  }

And the base class would look like this:

@AutoValue @FirebaseValue
public abstract class Tracker {
  public abstract String bar();

  public static JsonAdapter<Tracker> jsonAdapter(Moshi moshi) {
    return new AutoValue_Tracker.MoshiJsonAdapter(moshi);
  }

  public static Tracker fromDataSnapshot(DataSnapshot dataSnapshot) {
    return dataSnapshot.getValue(AutoValue_Tracker.FirebaseValue.class).toAutoValue();
  }

  public Object toFirebaseValue() {
    return new AutoValue_Tracker.FirebaseValue(this);
  }
}

I think I'll probably go down this road unless you have another suggestion. Thanks again for the detailed report!

f2prateek commented 8 years ago

Ah yup, sorry I mixed up my real class Tracker with an example class Foo.

That sounds like a reasonable solution to me!

mattlogan commented 8 years ago

Fixed in 0.2.0!

gabrielittner commented 8 years ago

Now it's never final. It should be final when isFinal is true and abstract otherwise.

mattlogan commented 8 years ago

Good point, thanks for report. That'll be an easy fix.

mattlogan commented 8 years ago

Fixed in version 0.2.1, here:

https://github.com/mattlogan/auto-value-firebase/blob/master/auto-value-firebase/src/main/java/me/mattlogan/auto/value/firebase/AutoValueFirebaseExtension.java#L87

Thanks again for the report.