JsonGenerator#writeTypePrefix writes an id "null" and cannot be set up to skip the id completely

[jonfreedman]

I have a TypeIdResolver implementation which works around generic type erasure to allow marshaling/unmarshaling of a parameterized type. This works fine when the underlying parameterized type maps directly onto a JSON type e.g. java.lang.Double but when it's a type that needs to be converted to a JSON string e.g. java.time.LocalDate then my TypeIdResolver is asked for a type and I can either return the String "java.time.LocalDate" or a null value.

If I return a null then TypeSerializerBase#handleMissingId currently does nothing as per FasterXML/jackson-databind#633 but it looks like the control of how to write the typeId is now handed off to JsonGenerator#writeTypePrefix @ https://github.com/FasterXML/jackson-core/blob/master/src/main/java/com/fasterxml/jackson/core/JsonGenerator.java#L1061

It would be great if writeTypePrefix did nothign if typeIdDef.id is null - I don't want to monkey-patch this unless it's going to make it into master...

[cowtowncoder]

Hmmh. I do not recall logic off-hand, but it seems to me that there is no point in attempting to write type id of null -- so perhaps caller should suppress it instead. But it also seems that if null was (allowed) to be passed, generator should ignore it. Although the other possible interpretation is that caller indicates that it has no idea of type id and lets generator determine if and what to write.
That is, change could be that if TypeIdResolver returns null, either:

1. Indicates "do not write type id", so call to generator should not be made, OR
2. Indicates resolver is not sure if (and what) id to use, so it lets generator handle this case best it can; and it is up to generator to either figure out id to use or skip.

I mention (2) mostly since the idea of making generator do more was to give more power to format-specific backends which know more about details of how to embed type ids (YAML, XML, Avro f.ex) and possible even changes to type ids.
I don't think I recall a case where caller would not know of a type id so it is probably not a use case.

So... I think we could consider blocking call (that is, approach (1)) for 2.11.

Do you have a sample use case that you think should work, but doesn't? I think I understand this scenario, but just want to double-check as this is bit intricate area.

[jonfreedman]

I'll speak to the relevant people at work to see if I can contribute something from the office otherwise I'll put something more generic together at home over the next few days. Do you have any tests in jackson-core that would be a good starting point for a failing test around this?

[cowtowncoder]

Unfortunately I think that this specific feature sort of falls between components so there might not be existing tests specifically targeting interaction. There are many tests for type id handling (in jackson-databind) but those operate at bit higher level, testing round-trip handling (writing polymorphic values, reading back).

But I think tests probably should go in jackson-databind.

[cowtowncoder]

Note to self: place where null id write could be blocked would be in TypeSerializerBase, lines 41+:

    @Override
    public WritableTypeId writeTypePrefix(JsonGenerator g,
            WritableTypeId idMetadata) throws IOException
    {
        _generateTypeId(idMetadata);
        return g.writeTypePrefix(idMetadata);
    }

where idMetadata.id == null check could be applied.

[jonfreedman]

We also likely need to have the same check in #writeTypeSuffix

[cowtowncoder]

@johnjohndoe Hmmh. Yes.

[johnjohndoe]

@cowtowncoder Typo-autocomplete?

[cowtowncoder]

@johnjohndoe Agh. Yes, thank you for pointing out.

@jonfreedman As per above... yes. :)

[beatfreaker]

@cowtowncoder are these changes correct in TypeSerializerBase ?
also any changes required in for deserialize process ?

public WritableTypeId writeTypePrefix(JsonGenerator g, WritableTypeId idMetadata) throws IOException {
    this._generateTypeId(idMetadata);
    if (idMetadata.id == null) {
        g.writeStartObject();
        return idMetadata;
    } else {
        return g.writeTypePrefix(idMetadata);
    }
}

public WritableTypeId writeTypeSuffix(JsonGenerator g, WritableTypeId idMetadata) throws IOException {
    if (idMetadata.id == null) {
        g.writeEndObject();
        return idMetadata;
    } else {
        return g.writeTypeSuffix(idMetadata);
    }
}

[cowtowncoder]

@beatfreaker I don't think use of g.writeStartObject() would be safe as starting marker can be START_ARRAY (for "as-array" method), or even "nothing at all" (in some cases type id is written before suffix). Logic is non-trivial, as you can see from writeTypeSuffix of JsonGenerator (in jackson-core).

Instead, this:

    public WritableTypeId writeTypePrefix(JsonGenerator g, WritableTypeId idMetadata) throws IOException
    {
        _generateTypeId(idMetadata);
        if (idMetadata.id == null) {
            return null;
        }
        return g.writeTypePrefix(idMetadata);
    }

    public WritableTypeId writeTypeSuffix(JsonGenerator g, WritableTypeId idMetadata) throws IOException
    {
        if (idMetadata.id == null) {
            return null;
        }
        return g.writeTypeSuffix(idMetadata);
    }

does pass all unit tests, but not sure if that would be desirable change.

[cowtowncoder]

@jonfreedman Ok it's been a while but as I am trying to close 2.12.0 feature set, was wondering if you had any further ideas here? As per my note above, simply skipping call on null id does pass all jackson-databind tests, but that's not high bar given that no test introduces null ids.

[cowtowncoder]

Ok, another year, not much progress alas (my fault). I am preparing for 2.13.1 patch release, so the likeliest chance to change this would be for 2.14.0 (for which there is branch but no firm release plans; progress slower than with 2.13).

[jonfreedman]

Thank you for keeping this on your radar

[cowtowncoder]

Created FasterXML/jackson-databind#3373 to implement.

Would REALLY appreciate a test of some kind, after implementation; will add notes on issue linked-to.

[cowtowncoder]

Implemented this on jackson-databind side: testing help / verification would be appreciated. Will be part of 2.14.0 release, so 2.14.0-SNAPSHOT (locally built or Sonatype OSS snapshots) should have the change.

[jonfreedman]

Unfortunately I no-longer have access to the codebase where I was using this, and I'm currently working in C# rather than Java... Based on my original issue I think the breaking test would be something along the lines of attempting to handle a custom type with type annotation and validating how null values are handled.

[cowtowncoder]

@jonfreedman Np. I think you did actually mention this. Appreciate all the help so far. Will see if someone else had similar needs or issues.

[cowtowncoder]

Will close for now, assume that databind-side fix resolves the issue. If not, we'll need a new issue to be filed, I think, with details of what is still needed etc.