Options on external enum constants are not linked

[damianw]

This might be a more general issue than just enum constants, and I also might be "holding it wrong", but I can't figure out if it's intentional that options on external enum constants are not linked.

The use case I have in mind is to encode metadata about particular enum constants directly in protobuf, and use this in a custom SchemaHandler. This doesn't seem to be possible unless I add some kind of stub to force Wire to link those types.

A test for OptionsLinkingTest.kt:

  @Test
  fun extensionTypesInExternalFileWithEnumOptions() {
    fs.add(
      "source-path/a.proto",
      """
        |syntax = "proto3";
        |
        |package src;
        |
        |import "extensions.proto";
        |import "range.proto";
        |
        |message B {
        |  option (lib.my_message_options) = {
        |    a: A_ONE
        |  };
        |}
      """.trimMargin()
    )
    fs.add(
      "proto-path/extensions.proto",
      """
        |syntax = "proto3";
        |
        |package lib;
        |
        |import "google/protobuf/descriptor.proto";
        |import "range.proto";
        |
        |message MyEnumValueOptions {
        |  Range length = 1;
        |}
        |
        |message MyMessageOptions {
        |  A a = 1;
        |}
        |
        |extend google.protobuf.EnumValueOptions {
        |  MyEnumValueOptions my_enum_value_options = 22002;
        |}
        |
        |extend google.protobuf.MessageOptions {
        |  MyMessageOptions my_message_options = 22002;
        |}
        |
        |enum A {
        |  A_UNSPECIFIED = 0;
        |  A_ONE = 1 [
        |    (my_enum_value_options) = {
        |      length: {
        |        min: 20
        |        max: 80
        |      }
        |    }
        |  ];
        |}
      """.trimMargin()
    )
    fs.add(
      "proto-path/range.proto",
      """
        |syntax = "proto3";
        |
        |package lib;
        |
        |message Range {
        |  double min = 1;
        |  double max = 2;
        |}
      """.trimMargin()
    )
    val schema = loadAndLinkSchema()

    // Assert "EnumValueOptions" type is present
    val enumValueOptionsType = schema.getType("google.protobuf.EnumValueOptions") as MessageType
    assertThat(enumValueOptionsType.extensionField("lib.my_enum_value_options")).isNotNull()

    // Assert option on enum constant is present
    val enumType = schema.getType("lib.A") as EnumType
    val value = assertNotNull(enumType.constant("A_ONE"))
    val valueOptionsMap = value.options.map
    val myEnumValueOptionsMember = ProtoMember.get(enumValueOptionsType.type, "lib.my_enum_value_options")
    assertThat(valueOptionsMap).containsOnlyKeys(myEnumValueOptionsMember)

    // Assert "MyEnumValueOptions" type is present
    val myEnumValueOptionsType = schema.getType("lib.MyEnumValueOptions") as MessageType
    assertThat(myEnumValueOptionsType.field("length")).isNotNull()

    // Assert "length" value is present
    @Suppress("UNCHECKED_CAST")
    val myValueOptionsMap = valueOptionsMap[myEnumValueOptionsMember] as Map<ProtoMember, Any?>
    val lengthMember = ProtoMember.get(myEnumValueOptionsType.type, "length")
    assertThat(myValueOptionsMap).containsOnlyKeys(lengthMember)

    // Assert "Range" type is present
    val rangeType = schema.getType("lib.Range") as MessageType
    assertThat(rangeType.field("min")).isNotNull()
    assertThat(rangeType.field("max")).isNotNull()

    // Assert "min" and "max" values are present
    @Suppress("UNCHECKED_CAST")
    val rangeMap = myValueOptionsMap[lengthMember] as Map<ProtoMember, Any?>
    val minMember = ProtoMember.get(rangeType.type, "min")
    val maxMember = ProtoMember.get(rangeType.type, "max")
    assertThat(rangeMap).containsOnlyKeys(minMember, maxMember)
    assertThat(rangeMap[minMember]).isEqualTo("20")
    assertThat(rangeMap[maxMember]).isEqualTo("80")
  }

This test passes if we add loader.loadExhaustively = true to loadAndLinkSchema, but fails without this flag:

java.lang.NullPointerException
    at com.squareup.wire.schema.Options.getMap(Options.kt:41)
    at com.squareup.wire.schema.OptionsLinkingTest.extensionTypesInExternalFileWithEnumOptions(OptionsLinkingTest.kt:290)

The fact that in a test this manifests as a NullPointerException in a test indicates to me that it is likely not intended behavior, and it was expected for the types in question to have been linked.

[oldergod]

I think this works as expected, doesn't it?

  /**
   * If true, the schema loader will load the whole graph, including files and types not used by
   * anything in the source path.
   */
  var loadExhaustively: Boolean

In the source path, the option you're trying to fetch isn't used.

Does it mean that loadExhaustively cannot be set to true when using the Wire Gradle plugin? We might able to change that if that can help? Could you not just add all in source-path instead of keeping some in proto path?

[damianw]

I think it kind of depends on how to interpret "being used". The type containing the options is being used, but the options on that type aren't preserved. That seems like something that I would expect to be preserved, since there's not an obvious other way besides what I'd call "workarounds", but that opinion based mostly on my use case.

Does it mean that loadExhaustively cannot be set to true when using the Wire Gradle plugin? We might able to change that if that can help?

That's right, and having that option would definitely help.

Could you not just add all in source-path instead of keeping some in proto path?

That's an option as well though I haven't totally explored it yet. It would require filtering on the generators, though.

[oldergod]

The way Wire sees things is that proto-path is only a context helper, used to resolve and validate all types/imports/etc in the source-path. It does look to me that you want everything to be in source-path? Maybe see if that's doable for you, and let us know if that becomes a problem and why.

[damianw]

Gotcha, thanks - that makes sense. I'll look into that. It seems that this is WAI, so you can feel free to close this issue. (Aside from the lack of ability to set loadExhaustively in either Gradle configuration or even programmatically so maybe that is the main take-away for this project?)

[oldergod]

I'd be down to add it if this solves a problem that couldn't otherwise be addressed.