[ProtoBuf]always call checkEnd() when skip unknown field

[marsqing]

We encounter a bug when _skipUnknownField(). The types involved are a little bit complicated and I'll submit a test case if necessary when I manage to simplify the types.

The problem is checkEnd() is not called when _state != STATE_NESTED_KEY, and tag = _decodeVInt(); on line 927 will be called. If the skipped field is the last field of an object, line 927 will decode a field of another object and try to find the field in the currently ended object on line 931.

[cowtowncoder]

Seems legit; test would be great of course, but in the meantime I think I'll backport this in 2.8(.11), 2.9(.3).

[cowtowncoder]

Hmmh. Actually, since this method is called from a few places, I think I would want a verification first to understand where this fails. By quick look I am guessing this could be problematic for an array-valued field? It should not, however, apply to root-level values. So I am bit worried about possibility of false alarms for root values, although it may be that this is not a problem since maybe end check value is Integer.MAX_VALUE...

In your specific case, what is the state value where this problem occurs? STATE_ARRAY_VALUE_PACKED or STATE_ARRAY_VALUE_OTHER perhaps?

I just want to understand specific case, and maybe even split method _checkEnd() if necessary.

[cowtowncoder]

Ok: I went ahead and manually merged the fix in 2.8 (I wish I knew how to better merge these) -- I think it's fine and should go in 2.9.3. Test would be nice of course but need not block it.

[marsqing]

Unit test to reproduce. Hope it can help you better fix the bug.
There's an extra field in EmbedV2. When deserializing OuterV2 which has an EmbedV2 field to Outer which has an Embed field, _skipUnknownField() will be called to skip the extra field. The _skipUnknownField() will also skip the state field of Outer, which will make the last assert fail.

import static org.junit.Assert.assertEquals;

import com.fasterxml.jackson.annotation.JsonAutoDetect;
import com.fasterxml.jackson.annotation.JsonAutoDetect.Visibility;
import com.fasterxml.jackson.annotation.JsonProperty;
import com.fasterxml.jackson.annotation.JsonPropertyOrder;
import com.fasterxml.jackson.core.JsonParser.Feature;
import com.fasterxml.jackson.databind.ObjectMapper;
import com.fasterxml.jackson.dataformat.protobuf.ProtobufMapper;
import com.fasterxml.jackson.dataformat.protobuf.schema.ProtobufSchema;
import com.fasterxml.jackson.dataformat.protobuf.schemagen.ProtobufSchemaGenerator;
import java.io.ByteArrayInputStream;
import java.io.ByteArrayOutputStream;
import java.util.Arrays;
import java.util.List;
import org.junit.Test;

public class BugReport {

  @Test
  public void test() throws Exception {
    // init mapper
    ProtobufMapper mapper = new ProtobufMapper();
    mapper.enable(Feature.IGNORE_UNDEFINED);
    ProtobufSchema schema = getSchema(mapper, Outer.class);
    ProtobufSchema schemaV2 = getSchema(mapper, OuterV2.class);


    // prepare value
    EmbedV2 embedV2 = new EmbedV2();
    embedV2.c = Arrays.asList("c");
    embedV2.extraField = "extra";
    
    OuterV2 v2Expected = new OuterV2();
    v2Expected.embed = embedV2;
    v2Expected.state="state";


    // serialize type with extra field
    ByteArrayOutputStream bout = new ByteArrayOutputStream();
    mapper.writer(schemaV2).writeValue(bout, v2Expected);

    showBytes(bout.toByteArray());

    // deserialize type with extra field
    OuterV2 v2Actual = mapper.readerFor(OuterV2.class).with(schemaV2).readValue(new ByteArrayInputStream(bout.toByteArray()));
    // this is ok
    assertEquals(v2Expected.state, v2Actual.state);

    // deserialize type without extra field
    Outer v1Actual = mapper.readerFor(Outer.class).with(schema).readValue(new ByteArrayInputStream(bout.toByteArray()));

    // Outer.state is skipped when skipping Embed.extraField
    assertEquals(v2Expected.state, v1Actual.state);
  }

  private ProtobufSchema getSchema(ObjectMapper mapper, Class<?> clazz) throws Exception {
    ProtobufSchemaGenerator gen = new ProtobufSchemaGenerator();
    mapper.acceptJsonFormatVisitor(clazz, gen);
    return gen.getGeneratedSchema();
  }

  private void showBytes(byte[] bytes) {
    for (byte b : bytes) {
      System.out.print(String.format("%8s", Integer.toHexString(b)).substring(6, 8).replaceAll(" ", "0") + " ");
    }
    System.out.println();
  }

  @JsonAutoDetect(
      getterVisibility = Visibility.NONE,
      setterVisibility = Visibility.NONE,
      isGetterVisibility = Visibility.NONE
  )
  @JsonPropertyOrder({"embed", "state"})
  public static class OuterV2 {
    @JsonProperty("embed")
    public EmbedV2 embed;
    @JsonProperty("state")
    public String state;
  }

  @JsonAutoDetect(
      getterVisibility = Visibility.NONE,
      setterVisibility = Visibility.NONE,
      isGetterVisibility = Visibility.NONE
  )
  @JsonPropertyOrder({"embed", "state"})
  public static class Outer {
    @JsonProperty("embed")
    public Embed embed;
    @JsonProperty("state")
    public String state;
  }

  @JsonAutoDetect(
      getterVisibility = Visibility.NONE,
      setterVisibility = Visibility.NONE,
      isGetterVisibility = Visibility.NONE
  )
  @JsonPropertyOrder({"a", "b", "c", "extraField"})
  public static class EmbedV2 {
    @JsonProperty("a")
    public String a;
    @JsonProperty("b")
    public String b;
    @JsonProperty("c")
    public List<String> c;
    @JsonProperty("extraField")
    public String extraField;
  }

  @JsonAutoDetect(
      getterVisibility = Visibility.NONE,
      setterVisibility = Visibility.NONE,
      isGetterVisibility = Visibility.NONE
  )
  @JsonPropertyOrder({"a", "b", "c"})
  public static class Embed {
    @JsonProperty("a")
    public String a;
    @JsonProperty("b")
    public String b;
    @JsonProperty("c")
    public List<String> c;
  }
}

[cowtowncoder]

Thank you!

[cowtowncoder]

Ok looks good: state is STATE_ARRAY_END, but I figured it's not clear if check could ever cause problems so I'll use patch as is. Thank you again for reporting this, as well as providing fix and test case.