Send data error handling

[JaroslawLegierski]

We noticed the problem in Leshan with the data received by the server using send data. For instance if the declared model is a Boolean and the data sent is “42”. Currently, it leads to a log error and a stack trace (lwm2m o.e.l.core.californium.LwM2mCoapResource : CodecException: Invalid content X for type X for path X).
We suggest that in this case the server should respond with 400 Bad request instead of 500 server error.

[sbernard31]

Here we need some javadoc to explain when we get this error and this will help to better define the behavior.

I mean when should we call this ? for any error or just when we think device send bad request ?

Maybe we should also replace InvalidRequestException by Exception ?

[JaroslawLegierski]

Replacing InvalidRequestException by Exception is good idea I also added a description in javadoc.

[sbernard31]

Reading your javadoc I understand that on Error should be called on any error.

So should we adapt the code for something llke :

 @Override
    public void handlePOST(CoapExchange exchange) {
        Request coapRequest = exchange.advanced().getRequest();
        Identity sender = extractIdentity(coapRequest.getSourceContext());
        Registration registration = registrationStore.getRegistrationByIdentity(sender);

        // check we have a registration for this identity
        if (registration == null) {
            exchange.respond(ResponseCode.NOT_FOUND, "no registration found");
            return;
        }

        try {
            // Decode payload
            LwM2mModel model = modelProvider.getObjectModel(registration);
            byte[] payload = exchange.getRequestPayload();
            ContentFormat contentFormat = ContentFormat.fromCode(exchange.getRequestOptions().getContentFormat());
            if (!decoder.isSupported(contentFormat)) {
                exchange.respond(ResponseCode.BAD_REQUEST, "Unsupported content format");
                return;
            }
            Map<LwM2mPath, LwM2mNode> data = null;

            data = decoder.decodeNodes(payload, contentFormat, (List<LwM2mPath>) null, model);

            // Handle "send op request
            SendRequest sendRequest = new SendRequest(contentFormat, data, coapRequest);
            SendableResponse<SendResponse> sendableResponse = sendHandler.handleSend(registration, sendRequest);
            SendResponse response = sendableResponse.getResponse();

            // send reponse
            if (response.isSuccess()) {
                exchange.respond(toCoapResponseCode(response.getCode()));
                sendableResponse.sent();
                return;
            } else {
                exchange.respond(toCoapResponseCode(response.getCode()), response.getErrorMessage());
                sendableResponse.sent();
                return;
            }
        } catch (RuntimeException e) {
            if (e instanceof CodecException) {
                InvalidRequestException invalidreqexception = new InvalidRequestException(e);
                sendHandler.onError(registration, invalidreqexception);
                throw invalidreqexception;
            } else {
                sendHandler.onError(registration, e);
                throw e;
            }
        }
    }

But I'm not so sure... if we need to do that ... 🤔
E.g. Why do we currently call onError on "payload error" but not on "content format error" ?

In your case, Do you need the onError ? or do you implement it because of the TODO you found in code ?
If you need it could you describe how ? maybe this could help to better define which behavior we must implement.

[JaroslawLegierski]

We would like first of all to handle the situation when content format is correct (eg SENML_CBOR) but we have mistake in data e.g. Boolean data is sent as String or Integer. We'd like to detect such situations. Why onError ? - because we were asked by colleagues who integrate Leshan with the external management system to implement this method in SendListener.

[sbernard31]

Why onError ? - because we were asked by colleagues who integrate Leshan with the external management system to implement this method in SendListener.

Ok that doesn't really help to better understand the need.

I guess, it makes sense to also raise en error about content format too.

(Just to let you know I will be unavailable until 5th January)

[sbernard31]

Yep something like this.

[JaroslawLegierski]

Thank You very much !
I tested it and recognized that sendHandler.onError method isn't always reached, for example when payload is incorrect: (byte[] payload = new byte[]{0x00, 0x10};) we got an error from org.eclipse.leshan.core.californium.LwM2mCoapResource - Exception while handling request because of exception during processing coapRequest.
When I removed coapRequest from InvalidRequestException :

            ...
		} catch (CodecException e) {

            exchange.respond(ResponseCode.BAD_REQUEST, "Invalid Payload");
            sendHandler.onError(registration,new InvalidRequestException(e, "Invalid payload from %s",sender));
            return;
        } catch (RuntimeException e) {
            sendHandler.onError(registration, e);
            throw e;
        }

sendHandler.onError is called correctly.

[sbernard31]

I'm not sure I get it.

You face an issue using #1195 ?
onError is not called correctly because of usage of coapRequest in the log ?
And so if we remove coapRequest from the log this is OK ?
That means there is a bug in Request.toString() from californium ?

Or I miss something ?

[sbernard31]

OK I got it this is a missing s after % at https://github.com/eclipse/leshan/blob/56d89325302011065971a427636d932e991cd677/leshan-server-cf/src/main/java/org/eclipse/leshan/server/californium/send/SendResource.java#L108

I will fix it.

Thx you very much for testing this and reporting the error 🙏.

[sbernard31]

I pushed the fix and 1 integration tests about that at #1195.

If you can confirm it works for you. (Do not hesitate to comment at #1195 instead of here)

[sbernard31]

@JaroslawLegierski thx for the contribution 🙏

I created a PR based on your work with some improvement. #1195
Could you review / test if that works for you ?

[sbernard31]

This PR is included in #1195.