Improve VKeyConverter

[hstonec]

This PR added an annotation processor to generate AttributeConoverter Java class for the entity class annotated with WithStringVKey or WithLongVKey. The generated class will be in the same package as the entity class, and its name will be the VKeyConverter_ concatenated with the type name of the VKey(or the value of classNameSuffix if set).

This change is 

[mindhog]

Reviewed 11 of 11 files at r1.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @hstonec)

---

core/src/main/java/google/registry/persistence/VKey.java, line 65 at r1 (raw file):

    checkArgumentNotNull(kind, "kind must not be null");
    checkArgumentNotNull(sqlKey, "sqlKey must not be null");
    checkArgumentNotNull(ofyKey, "ofyKey must not be null");

I seem to recall passing null to one of these in one place. That should break a test, but to be sure can you check the callsites?

---

core/src/main/java/google/registry/persistence/converter/LongVKeyConverterBase.java, line 34 at r1 (raw file):

    return dbData == null
        ? null
        : VKey.createSql(new TypeInstantiator<T>(getClass()) {}.getExactType(), dbData);

Once again, I think we can do this a lot better by generating the specialization classes using an annotation. Less runtime overhead, and self-documenting.

---

core/src/main/java/google/registry/persistence/converter/VKeyConverters.java, line 25 at r1 (raw file):

  @Converter(autoApply = true)
  public static class HostResourceVKeyConverter extends StringVKeyConverterBase<HostResource> {}

As I said in the other PR, I think the best place to put these is in the classes that they are relevant to (in this case, HostResource). But with an annotation we can simply annotate those classes to generate them (albeit not scoped to the class)

---

core/src/test/java/google/registry/persistence/VKeyTest.java, line 37 at r1 (raw file):

  @Test
  public void testOptionalAccessors() {
    VKey<TestObject> key = VKey.create(TestObject.class, null, null);

Ah yes. This place :-)

[hstonec]

Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @hstonec and @mindhog)

---

core/src/main/java/google/registry/persistence/VKey.java, line 65 at r1 (raw file):

Previously, mindhog (Michael Muller) wrote…

I seem to recall passing null to one of these in one place. That should break a test, but to be sure can you check the callsites?

Yeah, I modified the test.

---

core/src/main/java/google/registry/persistence/converter/LongVKeyConverterBase.java, line 34 at r1 (raw file):

Previously, mindhog (Michael Muller) wrote…

Once again, I think we can do this a lot better by generating the specialization classes using an annotation. Less runtime overhead, and self-documenting.

ok, let me try it to see how it looks like.

---

core/src/test/java/google/registry/persistence/VKeyTest.java, line 37 at r1 (raw file):

Previously, mindhog (Michael Muller) wrote…

Ah yes. This place :-)

Yeah.

[mindhog]

Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @hstonec and @mindhog)

---

core/src/main/java/google/registry/persistence/converter/LongVKeyConverterBase.java, line 34 at r1 (raw file):

Previously, hstonec (Shicong Huang) wrote…

ok, let me try it to see how it looks like.

Take a look at the "enum-set-ann" branch of my fork: https://github.com/mindhog/nomulus

It's generates a converter for EnumSet, but it's the same idea. The key things you need are:

• a "processors" subproject
• the annotationProcessor dependencies added to core/build.gradle
• settings.gradle (lists the subproject)
• processors/build.gradle
• processor/src/main/resources/META-INF/services/javax.annotation.processing.Processor

[hstonec]

Reviewable status: 3 of 44 files reviewed, 4 unresolved discussions (waiting on @mindhog)

---

core/src/main/java/google/registry/persistence/converter/VKeyConverters.java, line 25 at r1 (raw file):

Previously, mindhog (Michael Muller) wrote…

As I said in the other PR, I think the best place to put these is in the classes that they are relevant to (in this case, HostResource). But with an annotation we can simply annotate those classes to generate them (albeit not scoped to the class)

Done.

---

core/src/main/java/google/registry/persistence/converter/LongVKeyConverterBase.java, line 34 at r1 (raw file):

Previously, mindhog (Michael Muller) wrote…

Take a look at the "enum-set-ann" branch of my fork: https://github.com/mindhog/nomulus

It's generates a converter for EnumSet, but it's the same idea. The key things you need are:

• a "processors" subproject
• the annotationProcessor dependencies added to core/build.gradle
• settings.gradle (lists the subproject)
• processors/build.gradle
• processor/src/main/resources/META-INF/services/javax.annotation.processing.Processor

This is done.

[mindhog]

---

core/src/main/java/google/registry/persistence/LongVKey.java, line 21 at r2 (raw file):

import javax.persistence.AttributeConverter;

/** Annotation for {@link VKey} that is stored as a long in the database. */

I had envisioned this being applied to the class that we're generating a VKeyConverter for, for example:

@WithStringVKey
class ContactResource ...

Did you try that?

If we have to annotate every VKey field it kind of defeats the purpose of the annotation, which is to reduce the amount of boilerplate.

[mindhog]

---

processor/src/main/java/google/registry/processors/AbstractVKeyProcessor.java, line 130 at r2 (raw file):

      String converterClassName,
      TypeMirror vKeyTypeMirror,
      TypeMirror entityTypeMirror) {

This seems to be generating the converter - can we instead generate the specialization of the converter that just defines the getAttributeClass() method?

[mindhog]

---

processor/src/main/java/google/registry/processors/AbstractVKeyProcessor.java, line 25 at r2 (raw file):

import com.squareup.javapoet.ParameterizedTypeName;
import com.squareup.javapoet.TypeName;
import com.squareup.javapoet.TypeSpec;

I know javapoet is generally the prescribed way to do this, but it seems like for very simple cases it just complicates the code. Is there a reason you prefer it here?

[hstonec]

Reviewable status: 3 of 44 files reviewed, 7 unresolved discussions (waiting on @mindhog)

---

core/src/main/java/google/registry/persistence/LongVKey.java, line 21 at r2 (raw file):

Previously, mindhog (Michael Muller) wrote…

I had envisioned this being applied to the class that we're generating a VKeyConverter for, for example:

@WithStringVKey
class ContactResource ...

Did you try that?

If we have to annotate every VKey field it kind of defeats the purpose of the annotation, which is to reduce the amount of boilerplate.

I thought about that but was not sure if it works for things like VKey<? extends Entity>, but since you mention it I will try to see how it looks like.

---

processor/src/main/java/google/registry/processors/AbstractVKeyProcessor.java, line 25 at r2 (raw file):

Previously, mindhog (Michael Muller) wrote…

I know javapoet is generally the prescribed way to do this, but it seems like for very simple cases it just complicates the code. Is there a reason you prefer it here?

Even though it is not a very complicated class, I still feel javapoet is useful because it helps add the imports automatically, format the code, and most importantly it provides convenient function to work with the annotation processor API so I don't need to write code to handle it by myself.

---

processor/src/main/java/google/registry/processors/AbstractVKeyProcessor.java, line 130 at r2 (raw file):

Previously, mindhog (Michael Muller) wrote…

This seems to be generating the converter - can we instead generate the specialization of the converter that just defines the getAttributeClass() method?

I think you mean to make the generated converter extend the existing VKeyConverter? Hmm, my original idea is to not make the generated code depend on another piece of code.

[mindhog]

---

processor/src/main/java/google/registry/processors/AbstractVKeyProcessor.java, line 130 at r2 (raw file):

Previously, hstonec (Shicong Huang) wrote…

I think you mean to make the generated converter extend the existing VKeyConverter? Hmm, my original idea is to not make the generated code depend on another piece of code.

So, generally speaking, I've found that generated code is more difficult to deal with than normal code. For this reason, it's often useful to generate only the minimal set of code that needs to be parameterized and move everything else into non-generated, common code.
Why is it that you wish to avoid dependencies?

[mindhog]

Reviewed 42 of 42 files at r2.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @hstonec and @mindhog)

[hstonec]

Reviewable status: 33 of 44 files reviewed, 3 unresolved discussions (waiting on @mindhog)

---

processor/src/main/java/google/registry/processors/AbstractVKeyProcessor.java, line 130 at r2 (raw file):

Previously, mindhog (Michael Muller) wrote…

So, generally speaking, I've found that generated code is more difficult to deal with than normal code. For this reason, it's often useful to generate only the minimal set of code that needs to be parameterized and move everything else into non-generated, common code.
Why is it that you wish to avoid dependencies?

makes sense, I made the change.

[mindhog]

This rocks! Thanks!

Reviewed 10 of 11 files at r3.
Reviewable status: 43 of 44 files reviewed, 1 unresolved discussion (waiting on @hstonec)

---

core/src/main/resources/META-INF/persistence.xml, line 54 at r3 (raw file):

    <!-- Generated converters for VKey -->
    <class>google.registry.model.host.VKeyConverter_HostResource</class>

Gah!! Silly hibernate. [no action needed]