Can not write a field name, expecting a value

[sanderkoenders]

When we switched from Jackson version 2.8.9 to 2.9.8 we ran into some trouble with serializing data in CBOR format. As it turns out the custom Jackson module we've been using is causing trouble in this newer version. These issues start at version 2.9.0.

I've made a repository that reproduces the issue and found that when I disabled our custom TemporalAmountModule issues went away. I did not write the TemporalAmountModule myself but it looks like the Custom Serializer and Deserializer are the cullprit. These do not introduce any complex code but I don't really understand why they don't work properly anymore because I am not to familiar with Serializers and Deserializers.

I've made a repository that reproduces the issue I am having. When you disable the module you can see it is working properly. When you change the jackson version to 2.8.x (latest in that range is 2.8.11) you can see it works properly too.

https://github.com/Archcry/reproduce-jackson-error

The expected result should be the same as it was in version 2.8.9 (or 2.8.11 for that matter). Jackson should not break any functionality in a minor version release but it may deprecate certain methods (which they did). The deprecated methods are not the ones causing the issues though because I tried updating them but ended up with the same result. The actual result at the moment is an error saying Can not write a field name, expecting a value.

See also: https://stackoverflow.com/questions/56018579/can-not-write-a-field-name-expecting-a-value

[GedMarc]

public class TemporalAmountModule extends SimpleModule {
    private static final long serialVersionUID = 2316104230749384170L;

    public TemporalAmountModule() {
        this.addSerializer(TemporalAmount.class, new TemporalAmountSerializer());
        this.addDeserializer(TemporalAmount.class, new TemporalAmountDeserializer());
    }

    public static class TemporalAmountSerializer extends StdSerializer<TemporalAmount> {
        private static final long serialVersionUID = 2821083307561785524L;

        public TemporalAmountSerializer() {
            super(TemporalAmount.class);
        }

        @Override
        public void serialize(TemporalAmount value, JsonGenerator gen, SerializerProvider serializers) throws IOException {
            gen.writeString(value.toString());
        }

        @Override
        public void serializeWithType(TemporalAmount value, JsonGenerator gen, SerializerProvider serializers, TypeSerializer typeSer) throws IOException {
         //   typeSer.writeTypePrefixForScalar(value, gen);
            serialize(value, gen, serializers);
          //  typeSer.writeTypeSuffixForScalar(value, gen);
        }
    }

    public static class TemporalAmountDeserializer extends StdDeserializer<TemporalAmount> {
        private static final long serialVersionUID = 7929255530818556861L;

        public TemporalAmountDeserializer() {
            super(TemporalAmount.class);
        }

        @Override
        public TemporalAmount deserialize(JsonParser p, DeserializationContext ctxt) throws IOException {
            return parseDuration(p.getValueAsString());
        }
    }

    private static TemporalAmount parseDuration(String text) {
        Objects.requireNonNull(text, "text");

        if (text.contains("T")) {
            return Duration.parse(text);
        }
        else {
            return Period.parse(text);
        }
    }
}

[GedMarc]

turns out it was the deprecated fields -- not need to prefix and suffix values in 2.9

https://github.com/GedMarc/reproduce-jackson-error

[sanderkoenders]

Thanks, now it works :D Still weird this broke in a minor version though.

[cowtowncoder]

Would be great to understand what part exactly changed/failed, but I will say that breakage for existing code across minor versions is obviously not intended.

From exception message itself I can just speculate that usage may have been something that was not meant to be supported (nor expected to work) but happened to work. One example would be that once upon a time writing of JsonToken.VALUE_STRING in place where JsonToken.FIELD_NAME was (unintentionally) allowed, and for some format backends that would work (but not all). In such cases verification is sometimes added when problem is noticed, often as a side-effect of some other bug. I do not know if this was the case here, nor do I suggest that user would have been in the wrong since documentation/javadocs do not always clearly explain what is supported. And it is not unreasonable to verify actual behavior with implementation, especially in cases where definition is vague.

I'll have a look at @GedMarc 's repro, thanks!

[GedMarc]

Oh I deleted it already!!

Change was removing the start and stop prefixes in the serializer as above, and removing the time module which would get used for temporals instead of his custom one.

The writer crashed saying the input object was an array and not an object due to the implementation is his custom module.

Sorry for deleting too early!

On 7 May 2019 17:31, Tatu Saloranta notifications@github.com wrote:

Would be great to understand what part exactly changed/failed, but I will say that breakage for existing code across minor versions is obviously not intended.

From exception message itself I can just speculate that usage may have been something that was not meant to be supported (nor expected to work) but happened to work. One example would be that once upon a time writing of JsonToken.VALUE_STRING in place where JsonToken.FIELD_NAME was (unintentionally) allowed, and for some format backends that would work (but not all). In such cases verification is sometimes added when problem is noticed, often as a side-effect of some other bug. I do not know if this was the case here, nor do I suggest that user would have been in the wrong since documentation/javadocs do not always clearly explain what is supported. And it is not unreasonable to verify actual behavior with implementation, especially in cases where definition is vague.

I'll have a look at @GedMarchttps://nam02.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2FGedMarc&data=02%7C01%7C%7C1743f9abffc944936dd508d6d301068b%7C84df9e7fe9f640afb435aaaaaaaaaaaa%7C1%7C0%7C636928398685671270&sdata=lglk2wSIsA%2FlMkUv7ZW3ZXj70yWNWaBus3e3iMCS%2FQI%3D&reserved=0 's repro, thanks!

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHubhttps://nam02.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2FFasterXML%2Fjackson-databind%2Fissues%2F2320%23issuecomment-490130069&data=02%7C01%7C%7C1743f9abffc944936dd508d6d301068b%7C84df9e7fe9f640afb435aaaaaaaaaaaa%7C1%7C0%7C636928398685681281&sdata=66%2Fs%2BOat0Na1i2uzAZ%2BL0gXtytAMD%2B%2F3JBCTRPkwSmU%3D&reserved=0, or mute the threadhttps://nam02.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Fnotifications%2Funsubscribe-auth%2FABI6NWLPNMW73FRLPK5YKB3PUGODJANCNFSM4HLGZFPQ&data=02%7C01%7C%7C1743f9abffc944936dd508d6d301068b%7C84df9e7fe9f640afb435aaaaaaaaaaaa%7C1%7C0%7C636928398685701303&sdata=0ZER3TxD8KFg8cuqxGpzGpLE33yF1lz1UdD9kjDeQyQ%3D&reserved=0.

[cowtowncoder]

@GedMarc Ok yeah, just noticed. Was looking at the earlier link and from that it seemed as if code should have worked, even with deprecated method calls. But I'll keep on digging. :)

[cowtowncoder]

Odd. From what I see, the code SHOULD still work. I think I'll reopen this and see if I can figure out a root cause to support usage with deprecated calls. I think mistake here from my part was that test code was upgraded to not use any deprecated methods, which leaves old usage untested.

Also: not CBOR-specific, happens with JSON too (as I'd expect).

[cowtowncoder]

@Archcry one alternate minor suggestion, orthogonal to other things: use of StdScalarSerializer may make sense instead of bit more general StdSerializer. That would probably have avoided the issue as well.

[sanderkoenders]

Ye, as I said, I am not the one who wrote the code in the first place and I am not familiar with deserialization and serialization. I did find it odd that the code broke and I encourage you to dig deeper into it and maybe solve it in an upcoming release. Though I do think this issue is quite exotic because 2.9.x has been around for a while now and nobody else has had this issue. I'd say it's not a top priority. I might refactor our code when I have some time to boyscout. Thanks for the suggestions!

Edit: It would also be a good idea to revise the error message thrown in the code as it gave me no guidance on where to look for the issue. I've been debugging the issue for one day before finally deciding to make a reproduction repo and disable modules one by one to rule out the modules. I didn't quite expect it to be there but I was quite happy when I found out is was caused by our custom module because I finally had a lead to go from.

[cowtowncoder]

Unfortunately while I can see the exception from sample project, I am not able to construct simpler version with combinations. I wonder if that might be related to use of immutables, annotations. So I think I'll leave this as-is for now.

[cowtowncoder]

@Archcry right, breakage is unfortunate, and exception message unhelpful. Unfortunately at the point where call is validated is usually deep down in generator and usually has little context. Same is true for serializer implementations: custom (de)serializers are considered bit of advanced functionality and has fewer guard rails. In fact, often problems can even manifest at a later point when a broken (de)serializer has left things in inconsistent state.

I agree with it being interesting that I haven't seen other reports for 2.9.x. I hope I can find something eventually.

[sanderkoenders]

Thank you got he help :D

[sanderkoenders]

After having experienced some more issues with the solution @GedMarc offered I decided to try out the StdScalarSerializer which already had the serializeWithType. After investigation the inner workings of that class it seems the actual solution would be to write it like this:

@Override
public void serializeWithType(TemporalAmount value, JsonGenerator gen, SerializerProvider serializers, TypeSerializer typeSer) throws IOException {
    WritableTypeId typeId = typeSer.writeTypePrefix(gen, typeSer.typeId(value, JsonToken.VALUE_STRING));
    serialize(value, gen, serializers);
    typeSer.writeTypeSuffix(gen, typeId);
}

As seen in: https://github.com/FasterXML/jackson-databind/blob/d2c083a6220f2875c97c29f4823d9818972511dc/src/main/java/com/fasterxml/jackson/databind/ser/std/StdScalarSerializer.java#L30-L46

This also works in the reproduction repository.

Edit: Upon further inspection it seems typeSer.typeId has side-effects: This is also why the deprecated methods do not work as intended anymore, because writeTypeSuffixForScalar uses _writeLegacySuffix(g, typeId(value, JsonToken.VALUE_STRING)); in the background where typeId causes the suffix to be another WritableTypeId.

[cowtowncoder]

Oh. So is the problem that the intended reuse of same WritableTypeId does not occur, and thereby wrapperWritten is left false? If so that's... obviously something I overlooked, but also something difficult to solve.

[sanderkoenders]

After some more debugging I found that typeSer.writeTypePrefix is actually the method that has the side-effects. When passing the typeId you'd expect it's not changed inside the method but in reality it is. Then when you make a new typeId for the suffix it does not correspond to the typeId for the prefix and it throws the error. When the reference for the typeId for both the prefix and the suffix is the same everything works fine because the typeId for the suffix is also changed in the typeSer.writeTypePrefix method.

This probably also means that the deprecated methods do not work at all in this scenario because they work the same way I described (two different typeIds).

[cowtowncoder]

Right. It is intended to be mutable as that is the only mechanism for prefix- and suffix-handlers to collaborate wrt what former has written for latter to complete. But you are right in that it is not then possible to keep earlier mechanisms working.

[sanderkoenders]

From a clean code perspective you might want to reconsider this. As demonstrated in this issue method side-effects (intended or unintentional) will cause confusion for the programmer. I am not saying this should be fixed though, but it might be worth the consideration.

[cowtowncoder]

@Archcry If I could think of a way, yes, but I do not see anything obvious. There are plenty other fires to fight, unfortunately, and I need to pick my fights essentially. If anyone else has solutions that do not break new functionality but would improve compatibility by old implementations, I am all ears of course. But I do not have time to dig much deeper here myself unfortunately.

[sanderkoenders]

Yes, that's understandable. And I do agree with you that this is basically a non-issue since no other reports about this issue are reported so it seems like I am the only one having this issue. Since it is solved for me I wouldn't pay to much attention to it. I also think the issue is well documented in this report so everyone else who has problems should be able to solve it via this report.

[cowtowncoder]

Yes, I concur. I wish I could do more (and who knows, maybe something turns up). But at least there is something documented here if others end up hitting this issue.