Better customization of Leshan server

[adamsero]

Hi, I wanted to discuss a better way we could structure Leshan code in order to make it easier to swap certain components for their custom implementations. As an example, I'll describe adding a custom Base64 encoder/decoder to SenMLJsonRecordSerDes class - this is a throwback to #1325 where we discussed new API for encoding and decoding.

Let's say that on our side we want to inject a custom Base64Encoder and Base64Decoder implementations. Customization starts with the LeshanServerBuilder, which has the required setter for new encoders:

leshanServerBuilder.setEncoder(buildPatchedEncoder());

Then we have to rebuild our own SenMLJsonJacksonEncoderDecoder:

private static LwM2mEncoder buildPatchedEncoder() {
    Map<ContentFormat, NodeEncoder> nodeEncoders = DefaultLwM2mEncoder.getDefaultNodeEncoders(false);
    nodeEncoders.put(ContentFormat.SENML_JSON, new LwM2mNodeSenMLEncoder(new SenMLJsonJacksonEncoderDecoder()));
    return new DefaultLwM2mEncoder(nodeEncoders, DefaultLwM2mEncoder.getDefaultPathEncoder(), new LwM2mValueChecker());
}

Note that there is almost no change in SenMLJsonJacksonEncoderDecoder code with respect to the Leshan version, except we inject a custom SenMLJsonRecordSerDes during construction:

public SenMLJsonJacksonEncoderDecoder(boolean allowNoValue) {
    this.serDes = new SenMLJsonRecordSerDes(allowNoValue);
}

And finally in our custom version of SenMLJsonRecordSerDes, we basically have 2 lines of code to change with respect to the Leshan version (related to OPAQUE fields encoding and decoding):

case OPAQUE:
    jsonObj.put("vd", urlSafeEncoder.encode(record.getOpaqueValue()));
    break;

and

JsonNode vd = o.get("vd");
if (vd != null && vd.isTextual()) {
    record.setOpaqueValue(urlSafeDecoder.decode(vd.asText()));
    hasValue = true;
}

This is a lot of code duplication in both SenMLJsonJacksonEncoderDecoder and SenMLJsonRecordSerDes, the only reason being that the Base64 decoder is not easily “reachable” during server config. That’s why it would be helpful to extend more widely the builder pattern that helps constructing the Leshan server.

As you know, the LeshanServerBuilder exposes setters for a number of internal components, and if no custom implementation is provided for a component, switches to default implementation when build() is called. Unfortunately this builder pattern does not exist for other components. For instance, it would be convenient to be able to do this in order to inject new encoders/decoders:

private static LwM2mEncoder buildPatchedEncoder() {
    Map<ContentFormat, NodeEncoder> nodeEncoders = DefaultLwM2mEncoder.getDefaultNodeEncoders(false);
    nodeEncoders.put(ContentFormat.SENML_JSON, new LwM2mNodeSenMLEncoder(
            new SenMLJsonJacksonEncoderDecoderBuilder()
                    .setSerDes(new SenMLJsonRecordSerDesBuilder()
                            .setBase64Encoder(new CustomSenMLBase64Encoder())
                            .setBase64Decoder(new CustomSenMLBase64Decoder())
                            .build())
                    .build()));
    return new DefaultLwM2mEncoder(nodeEncoders, DefaultLwM2mEncoder.getDefaultPathEncoder(), new LwM2mValueChecker());
}

With a SenMLJsonJacksonEncoderDecoderBuilder and a SenMLJsonRecordSerDesBuilder, there is no code duplication, and we are able to inject directly our own Base64 encoder/decoder. What do you think about this idea?

[sbernard31]

I totally agree that currently is is not so easy to tweak SenMLJsonJacksonEncoder
And it would be an improvement to avoid the duplicate code you mention. 👍

The remaining question is how to do that ?

Adding a new Interface for Base64 Encoder/Decoder make sense to me (more details at #1325 (comment)).

About create 2 new builders, I'm not sure.
The drawback of builder is that :

• you can not easily use it with inheritance. (Eg. if you extend a SenMLJsonRecordSerDes, this will not so sure that you will be able to use SenMLJsonRecordSerDesBuilder)
• this is also generally more code.

When you have lot of parameter and complicate configuration, it's generally easy to favor the builder pattern.
Here I'm not sure there is enough constructor argument to justify to add a new builder.

WDYT ?

[adamsero]

Yes it’s true that the builder pattern might be a bit cumbersome for those two classes, since they have very few dependencies and probably won't evolve too much in the future. In this case I guess constructor injection might be enough to meet the need. Something like:

private static LwM2mEncoder buildPatchedEncoder() {
    Map<ContentFormat, NodeEncoder> nodeEncoders = DefaultLwM2mEncoder.getDefaultNodeEncoders(false);
    nodeEncoders.put(ContentFormat.SENML_JSON, new LwM2mNodeSenMLEncoder(new SenMLJsonJacksonEncoderDecoder(
            new SenMLJsonRecordSerDes(new CustomBase64Encoder())
    )));
    return new DefaultLwM2mEncoder(nodeEncoders, DefaultLwM2mEncoder.getDefaultPathEncoder(), new LwM2mValueChecker());
}

Adding a few constructors seems light enough, for now. The only problem is that this pattern does not scale very well.

For instance, regarding SenMLJsonJacksonEncoderDecoder, it make sense to allow injection of a custom ObjectMapper as well, and there is already a allowNoValue flag in the current constructor. Combining the 3 possible customizations (serializer, mapper, allowNoValue) would make 9 constructors. We could reduce to 2 if we keep only the default contructor and a constructor with all 3 possible customizations, but it would require access to default implementations. Something like:

new SenMLJsonJacksonEncoderDecoder(
        new SenMLJsonRecordSerDes(new CustomBase64Encoder()),
        SenMLJsonJacksonEncoderDecoder.DEFAULT_OBJECT_MAPPER,
        SenMLJsonJacksonEncoderDecoder.DEFAULT_NO_VALUE_OPTION
)));

We could use Lombok for generic constructors, but I guess it's too late to integrate it in the code since we have many other builders, including some custom ones.

[sbernard31]

I didn't know lombol could you share a link ?

My first thought :

I feel 3 constructor is maybe ok.

The 2 existing one + one with all parameters.
I don't now if we really need default constant as no_value is just a boolean and default object mapper is just new ObjectMapper() ?

[adamsero]

I didn't know lombol lombok could you share a link ?

https://projectlombok.org/features/

[sbernard31]

https://projectlombok.org/features/

I prefer to not add a dependency just for this. (And as soon as you want to customize your builder, I guess this will be complicated)

[sbernard31]

Done with : #1372

[sbernard31]

#1372 integrated it in master should be available in 2.0.0-M10.