Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

JSON Pointer inconsistent when serializing first element of an array #1154

Open
itineric opened this issue Dec 5, 2023 · 5 comments
Open
Labels
need-test-case Needs a test case (unit test or such) to proceed

Comments

@itineric
Copy link

itineric commented Dec 5, 2023

During serialization, when using a custom serializer, the JsonPointer is inconsistent when serializing the first element. Instead of having /myList/0 we get /myList

This happens because we enter this part of code : https://github.com/FasterXML/jackson-core/blob/2.17/src/main/java/com/fasterxml/jackson/core/JsonPointer.java#L212

The call stack looks like this, the Object received in my CustomSerializer.serialize method is the first element of my array.

JsonPointer.forPath(JsonStreamContext, boolean) line: 215	
JsonWriteContext(JsonStreamContext).pathAsPointer() line: 279	
CustomSerializer.serializeWithType(Object, JsonGenerator, SerializerProvider, TypeSerializer)
IndexedListSerializer.serializeTypedContents(List<?>, JsonGenerator, SerializerProvider) line: 181	
IndexedListSerializer.serializeContents(List<?>, JsonGenerator, SerializerProvider) line: 92	
IndexedListSerializer.serialize(List<?>, JsonGenerator, SerializerProvider) line: 79	

Additional info: the problem seems to be the _index of _writeContext of JsonGenerator instance which is -1 but should be 0 at that time.

@cowtowncoder cowtowncoder added the need-test-case Needs a test case (unit test or such) to proceed label Dec 6, 2023
@cowtowncoder
Copy link
Member

While description suggest there is a problem, what is really needed is a reproduction as code (unit test).
It looks like this would be related to handling of polymorphic type id (Serializer.serializeWithType()) so it can be tricky to reproduce without knowing exact code and POJOs used.

@cowtowncoder cowtowncoder added the 2.17 Issues planned (at earliest) for 2.17 label Dec 6, 2023
@itineric
Copy link
Author

itineric commented Dec 6, 2023

I also have the issue with serialize. It does not depend on polymorphic types.

The simpliest I can come with is the following. It does not make any sense since the custom serializer juste calls a default one but its for unit testing purpose.

The custom serializer

public class JsonPointerKeeperSerializer extends StdSerializer<Object>
{
  private static final long serialVersionUID = 1L;

  List<String> _jsonPointers = new ArrayList<>();

  public JsonPointerKeeperSerializer()
  {
    super(Object.class);
  }

  @Override
  public void serialize(final Object value, final JsonGenerator gen, final SerializerProvider serializers)
    throws IOException
  {
    final JsonStreamContext outputContext = gen.getOutputContext();
    final String pointer = outputContext.pathAsPointer().toString();
    _jsonPointers.add(pointer);

    final JavaType javaType = serializers.constructType(value.getClass());
    final JsonSerializer<Object> serializer = BeanSerializerFactory.instance.createSerializer(serializers, javaType);
    serializer.serialize(value, gen, serializers);
  }
}

The unit test class

public class JsonPointerInSerializerTest
{
  private final ObjectMapper _objectMapper = new ObjectMapper();
  private final JsonPointerKeeperSerializer _jsonPointerKeeperSerializer = new JsonPointerKeeperSerializer();

  public JsonPointerInSerializerTest()
  {
    final SimpleModule module = new SimpleModule();
    module.addSerializer(Object.class, _jsonPointerKeeperSerializer);
    _objectMapper.registerModule(module);
  }

  @Test
  public void serializeDeserializeWithRefsInArrays() throws Exception
  {
    final Parent parent = new Parent();
    final Child firstChild = new Child();
    final Child secondChild = new Child();
    parent.getChilds().addAll(Arrays.asList(firstChild, secondChild));

    _objectMapper.writeValueAsString(parent);
    final List<String> jsonPointers = _jsonPointerKeeperSerializer._jsonPointers;
    assertEquals(Arrays.asList("", "/childs", "/childs/0", "/childs/1"),
                 jsonPointers);
  }
}

Parent class

public class Parent
{
  private final List<Child> childs = new ArrayList<>();

  public List<Child> getChilds()
  {
    return childs;
  }
}

Child class

public class Child
{
}

The result:
expected: <[, /childs, /childs/0, /childs/1]> but was: <[, /childs, /childs, /childs/0]

I understand why I get there, it is not considering the array has started until the first element was written. But inside serialize the passed value is indeed the first element of the array, so the JsonPointer is not consistent.

@cowtowncoder
Copy link
Member

Ok thanks. Reading through it all, I am not actually sure this is bug: JsonPointer refers to location of most recently written token; not to location of next token... I think. Although this is tricky area; when reading, the "current location" is easier to define, being the token just returned (jsonParser.currentToken()).

For writing I can see how it could be either current (just written) -- like currently -- or "next to write".

And changing this behavior would probably be breaking change for existing usage as well... assuming writer-side location is commonly used (I'd guess custom serializers do use it).
Handling is at low level, too, pathAsPointer() itself is simple, it's context that tracks location.

So. Not sure what to do here. I may have to consider this "working as-is".

@itineric
Copy link
Author

itineric commented Dec 7, 2023

I understand your explanation but it is still odd to be in a serialize method, receive an array element value as parameter and then get a JsonPointer that refers to the array itself.

@cowtowncoder
Copy link
Member

cowtowncoder commented Dec 7, 2023

@itineric Yeah usage of JsonPointer based location (or, location information in general) seems more complicated / less useful on writer than on read side.

One thing that might be helpful in figuring out things is to use JsonGenerator directly, to see how Output Context gets updated. I think it is done in response to various writeXxx() calls and is not -- unlike setting of "current value" (JsonGenerator.assignCurrentValue) -- updated by serializers at all.
This is important because whereas I think there are places where serializers are not updating "current value" correctly, I don't think there is much in the way writeXxx() methods are implemented that was incomplete or inconsistent (wrt sequence of calls expected -- not arguing about strange-ness (which I agree with) of resulting JsonPointer).

Having said all that; if there was a patch to add an alternate pathAsPointer() (different name) that tries to figure out more useful JsonPointer -- pathToNextValue()? -- I'd be happy to help get that merged.
The reason for different method has to do with backwards compatibility.

@cowtowncoder cowtowncoder removed 2.17 Issues planned (at earliest) for 2.17 labels Sep 11, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
need-test-case Needs a test case (unit test or such) to proceed
Projects
None yet
Development

No branches or pull requests

2 participants