Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(binding-coap): send error response if content negotiation fails #849

Merged
merged 2 commits into from
Nov 15, 2022

Conversation

JKRhb
Copy link
Member

@JKRhb JKRhb commented Oct 19, 2022

Dealing a bit more with CoAP content negotiation, I noticed in the RFC that instead of using a default Content-Format in the case of failed negotiation, an error response should must be sent. This PR takes that into account, adjusting the internal negotiateContentFormat method and the test. Since this caused the logic to become a bit more complicated, I added some documentation to hopefully clarify how the new, now RFC-compliant process works.

@JKRhb JKRhb force-pushed the content-negotiation-fix branch from f53965b to 42fc764 Compare October 19, 2022 12:44

if (negotiationSuccessful) {
res.code = "2.05";
res.end(JSON.stringify(thing.getThingDescription()));
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could the whole function could be simplified like this?

switch (accept) {
    case ContentSerdes.DEFAULT:
    case ContentSerdes.TD:
    case null:
        res.code = "2.05";
        res.setHeader("Content-Format", accept || ContentSerdes.TD);
        res.end(JSON.stringify(thing.getThingDescription()));
        break;
    default:
        res.code = "4.06";
        res.end(`Content-Format ${accept} is not supported by this resource.`);
        break;
}

Copy link
Member Author

@JKRhb JKRhb Oct 19, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I incorporated your suggestion in d1f3ae1. My initial thoughts were that this method could be a generic one for content negotiation, but maybe that is a bit too much. Having a specific method could make more sense, at least for now.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the generic solution would probably look like this:

let serializer = SERIALIZERS[accept];
if (serializer) {
    let content = serializer.serialize(thing.getThingDescription());
    res.code = "2.05";
    res.setHeader("Content-Format", content.Type);
    res.end(content.Data);
} else {
    res.code = "4.06";
    res.end(`Content-Format ${accept} is not supported by this resource.`);
}

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The latest commits now implement a generic version using the ContentSerdes class for serialization. In the future, this would also allow for different TD representations (e.g., as CBOR or XML), but it seems as if these two codecs are currently not supported/implemented.

@JKRhb
Copy link
Member Author

JKRhb commented Oct 19, 2022

Hmm, does somebody know why the Windows tests are suddenly failing? :/ Edit: My bad, it's actually the firestore tests Edit2: Or both actually?

@danielpeintner
Copy link
Member

it's actually the firestore tests Edit2: Or both actually?

we do have issues with firestore since a while (see #827)..

@JKRhb JKRhb force-pushed the content-negotiation-fix branch from 4f38a0a to cd40a84 Compare November 11, 2022 13:29
@JKRhb JKRhb force-pushed the content-negotiation-fix branch from cd40a84 to 1b8be90 Compare November 11, 2022 13:30
@JKRhb
Copy link
Member Author

JKRhb commented Nov 11, 2022

@danielpeintner Thank you for approving :) I took the freedom of squashing the commits.

Copy link
Member

@relu91 relu91 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Minor point below

packages/binding-coap/test/coap-server-test.ts Outdated Show resolved Hide resolved
@JKRhb JKRhb force-pushed the content-negotiation-fix branch from 1b8be90 to d85002c Compare November 12, 2022 11:50
@danielpeintner
Copy link
Member

Good to go from my side. I let @relu91 push the merge button.

@relu91 relu91 merged commit 0182f39 into eclipse-thingweb:master Nov 15, 2022
@JKRhb JKRhb deleted the content-negotiation-fix branch November 15, 2022 10:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants