square / square/wire

OptionReader fails to parse a parenthesized extension after a regular option path component

Open
#3,672 1 comment 0 reactions 0 assignees View on GitHub

Nobody has claimed this yet.

Dominant language
Kotlin
Stars
4.4k
Forks
627
Avg merge
3d 15m
Merged PRs (30d)
20

Description

Description

Wire's ProtoParser fails to parse a parenthesized extension when it follows a regular field component in a custom option path.

This fails:

(foo.field).string.(foo.datetime) = true

Adding whitespace before the second dot makes the same option parse successfully:

(foo.field).string .(foo.datetime) = true

This occurs in real-world Protovalidate predefined rules, for example:

(buf.validate.field).string.(company.validate.datetime) = true

Protovalidate documents this form of nested extension syntax:

(buf.validate.field).float.(foo.bar.required_with_max)

https://protovalidate.com/schemas/predefined-rules/#applying-predefined-rules

Environment

  • com.squareup.wire:wire-schema-jvm:5.4.0
  • Java 21.0.9
  • Reproduced by invoking ProtoParser directly
  • Initially encountered with confluentinc/cp-schema-registry:8.0.3, which bundles Wire 5.4.0

The failure does not require Schema Registry and can be reproduced by calling Wire directly.

Minimal reproduction

import com.squareup.wire.schema.Location;
import com.squareup.wire.schema.internal.parser.ProtoParser;

public final class WireParseTest {
    public static void main(String[] args) {
        String schema =
            "syntax = \"proto3\";\n" +
            "\n" +
            "message TestEvent {\n" +
            "  string started_at = 1 [\n" +
            "    (foo.field).string.(foo.datetime) = true\n" +
            "  ];\n" +
            "}\n";

        var parsed = ProtoParser.Companion.parse(
            Location.get("test.proto"),
            schema
        );

        System.out.println(parsed);
    }
}

Compile and execute it against Wire 5.4.0:

javac \
  -proc:none \
  -cp 'wire-schema-jvm-5.4.0.jar:<required-runtime-dependencies>' \
  WireParseTest.java

java \
  -cp '.:wire-schema-jvm-5.4.0.jar:<required-runtime-dependencies>' \
  WireParseTest

It can also be reproduced using the Schema Registry image that contains Wire 5.4.0:

docker run --rm \
  --volume "$PWD:/work" \
  --workdir /work \
  --entrypoint sh \
  confluentinc/cp-schema-registry:8.0.3 \
  -c '
    javac \
      -proc:none \
      -cp "/usr/share/java/schema-registry/*" \
      WireParseTest.java &&
    java \
      -cp "/work:/usr/share/java/schema-registry/*" \
      WireParseTest
  '

Actual result

Exception in thread "main" java.lang.IllegalStateException:
Syntax error in test.proto:5:34: expected '=' in option
    at com.squareup.wire.schema.internal.parser.SyntaxReader.unexpected(SyntaxReader.kt:425)
    at com.squareup.wire.schema.internal.parser.OptionReader.readOption(OptionReader.kt:215)
    at com.squareup.wire.schema.internal.parser.OptionReader.readOptions(OptionReader.kt:38)
    at com.squareup.wire.schema.internal.parser.ProtoParser.readField(ProtoParser.kt:369)
    at com.squareup.wire.schema.internal.parser.ProtoParser.readField(ProtoParser.kt:353)
    at com.squareup.wire.schema.internal.parser.ProtoParser.readDeclaration(ProtoParser.kt:168)
    at com.squareup.wire.schema.internal.parser.ProtoParser.readMessage(ProtoParser.kt:206)
    at com.squareup.wire.schema.internal.parser.ProtoParser.readDeclaration(ProtoParser.kt:150)
    at com.squareup.wire.schema.internal.parser.ProtoParser.readProtoFile(ProtoParser.kt:71)
    at com.squareup.wire.schema.internal.parser.ProtoParser$Companion.parse(ProtoParser.kt:680)
    at WireParseTest.main(WireParseTest.java:15)

Expected result

The option should parse successfully without requiring whitespace before the second dot:

(foo.field).string.(foo.datetime) = true

Whitespace around the dot should not change the meaning or validity of the option path.

Whitespace workaround

Changing only this:

(foo.field).string.(foo.datetime) = true

to this:

(foo.field).string .(foo.datetime) = true

makes Wire parse the schema successfully.

The equivalent Protovalidate message-literal syntax also works:

(buf.validate.field).string = {
  [company.validate.datetime]: true
}

Suspected cause

OptionReader.readOption() appears to support nested parenthesized option components:

val subName = reader.readName(retainWrap = true)

if (subName.startsWith("(")) {
  subNames.add(subName)
} else {
  subNames.addAll(subName.split("."))
}

However, SyntaxReader.readWord() treats . as part of a word:

when (data[pos]) {
  in 'a'..'z', in 'A'..'Z', in '0'..'9', '_', '-', '.' -> pos++
  else -> break@loop
}

When parsing:

(foo.field).string.(foo.datetime)

the call that should read string appears to consume string. instead. The separator dot before (foo.datetime) therefore never reaches the loop in OptionReader.

OptionReader subsequently encounters ( where it expects =, producing:

expected '=' in option

The successful whitespace workaround supports this analysis: whitespace prevents readWord() from consuming the separator dot as part of string..

Relevant sources:

Wire already contains support and tests for some nested extension options, such as:

option (my_message_option_six).(More.more_string) = "foobar";

However, that form does not contain a regular path component between two parenthesized extensions. The failing structure is specifically:

(parenthesized extension).regular field.(parenthesized extension)

Contributor guide

Open the contributing guide

First steps

  1. Read the whole issue, then the project's contributing guide.
  2. Comment on the issue to say you are picking it up — it saves two people doing the same work.
  3. Fork the repository and make your change on a branch.
  4. Open a pull request that references the issue number.

Research direction

Start with wire-schema/src/commonMain/kotlin/com/squareup/wire/schema/internal/parser/OptionReader.kt and SyntaxReader.kt, focusing on how regular and parenthesized option path components consume dots. Review the existing nested extension option tests, then add coverage for a parenthesized extension after a regular component. Done means (foo.field).string.(foo.datetime) = true parses successfully without requiring whitespace.

Written by the indexing model from the issue text.

Assessment

Tech stack
kotlin
Domain
compilers
Issue type
Bug
Difficulty
3/5
Estimated time
1-2 days
Activity status
Quiet
Clarity
Clearly specified
Newbie friendliness
76/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.