FasterXML / jackson-dataformats-binary

Uber-project for standard Jackson binary format backends: avro, cbor, ion, protobuf, smile
Apache License 2.0
310 stars 133 forks source link

IonValueMapper.builder() not implemented, does not register modules #509

Closed mr-robert closed 1 month ago

mr-robert commented 1 month ago

(follow-up from #497)

Using Jackson-dataformat-ion 2.17

When constructing a new IonValueMapper using constructor new IonValueMapper(onSystem) modules are registered:

Constructor code:

    public IonValueMapper(IonSystem ionSystem, PropertyNamingStrategy strategy) {
        super(new IonFactory((ObjectCodec)null, ionSystem));
        this.registerModule(new IonValueModule());
        this.registerModule(new EnumAsIonSymbolModule());
        this.setPropertyNamingStrategy(strategy);
    }

However, when constructing using builder: IonValueMapper.builder(ionSystem).build()

the modules are not registered, ultimately by chain of method calls, the following code is run in IonObjectMapper.class :

    public static Builder builder(IonFactory streamFactory) {
        return new Builder(new IonObjectMapper(streamFactory));
    }

this creates an IonObjectMapper , without registering IonValueModule or EnumAsIonSymbolModule.

I think that the correct/expected behavior should be that using IonValueMapper.builder(...) should ultimately create a mapper which is the same as using constructor with the same parameters. Therefore it seems that there may need to be some changes or additional builder overrides added in IonValueMapper.

Workaround for this is just to not use builder to create IonValueMapper , or manually registering the modules.

cowtowncoder commented 1 month ago

Ok this is bit tricky, due to static methods etc. But I will try to cobble something together. For Jackson 3.0 IonValueMapper is deprecated, replaced by proper builder of IonObjectMapper, fwtw.

cowtowncoder commented 1 month ago

@mr-robert Did pr #510, merged; not confident it fully works but if you have a chance maybe you could have a look (2.18.0-SNAPSHOT should be built by CI, or build locally)?