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

feat(coap-server): add initial meta operation support #1086

Merged
merged 4 commits into from
Sep 22, 2023

Conversation

JKRhb
Copy link
Member

@JKRhb JKRhb commented Sep 15, 2023

Based on the refactoring made in #1084, this PR adds initial support for meta operations, namely the reading of multiple or all properties, to the CoAP server. This corresponds with issue #1014 – as only one kind of meta operation is supported yet, I think the issue can be kept open for now.

I haven't figured out the correct way to implement writing multiple properties yet, which is why I left it out of this PR for now. However, a method skeleton already exists (at the moment, it simply returns an error response with code 5.01 (Not Implemented)) which can be expanded in a follow-up PR.

As this PR depends on #1084, I will rebase once that PR has been merged to make reviewing a bit easier. Until then, I will keep this PR in draft status.

@JKRhb JKRhb added the binding-coap Issues related to coap protocol binding label Sep 15, 2023
@codecov
Copy link

codecov bot commented Sep 15, 2023

Codecov Report

Patch coverage: 93.07% and project coverage change: +0.22% 🎉

Comparison is base (227be53) 75.31% compared to head (641cae5) 75.53%.
Report is 3 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1086      +/-   ##
==========================================
+ Coverage   75.31%   75.53%   +0.22%     
==========================================
  Files          80       80              
  Lines       16060    16172     +112     
  Branches     1503     1519      +16     
==========================================
+ Hits        12096    12216     +120     
+ Misses       3925     3917       -8     
  Partials       39       39              
Files Changed Coverage Δ
packages/binding-coap/src/coap-server.ts 81.19% <93.07%> (+2.82%) ⬆️

... and 2 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@egekorkan
Copy link
Member

I haven't figured out the correct way to implement writing multiple properties yet, which is why I left it out of this PR for now.

You are not alone at this so it can definitely be postponed: w3c/wot-thing-description#848 . However, wouldn't it be better to never generate a TD with it?

@JKRhb JKRhb marked this pull request as ready for review September 15, 2023 16:09
@JKRhb
Copy link
Member Author

JKRhb commented Sep 15, 2023

However, wouldn't it be better to never generate a TD with it?

Good point, I removed it for now alongside the handleWriteMultipleProperties method in order to not confuse people. However, note that in the HTTP server, the write meta operations are actually added to the op array despite having no corresponding handler method defined:

if (anyProperties) {
const href = base + "/" + this.PROPERTY_DIR;
const form = new TD.Form(href, type);
if (allReadOnly && !allWriteOnly) {
form.op = ["readallproperties", "readmultipleproperties"];
} else if (allWriteOnly && !allReadOnly) {
form.op = ["writeallproperties", "writemultipleproperties"];
} else {
form.op = [
"readallproperties",
"readmultipleproperties",
"writeallproperties",
"writemultipleproperties",
];
}
if (!thing.forms) {
thing.forms = [];
}
thing.forms.push(form);
this.addUrlRewriteEndpoints(form, thing.forms);
}

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.

Just a small comment below. Test coverage could be also improved but we can do it in a future PR.

Comment on lines +224 to +230
if (readableProperties > 0) {
opValues.push("readmultipleproperties");
}

if (readableProperties === numberOfProperties) {
opValues.push("readallproperties");
}
Copy link
Member

Choose a reason for hiding this comment

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

Actually, the spec is not clear about this semantic. One could still understand that the readallproperties operation just works on the set of readable properties of the thing. I guess for the time being is alright to leave as it is, but still other bindings implementations might act differently.

@danielpeintner danielpeintner merged commit aae70f4 into eclipse-thingweb:master Sep 22, 2023
7 checks passed
@JKRhb JKRhb deleted the coap-meta branch September 22, 2023 09:44
JKRhb added a commit to JKRhb/thingweb.node-wot that referenced this pull request Sep 22, 2023
…eb#1086)

* feat(coap-server): add support for property meta operations

* fixup! feat(coap-server): add support for property meta operations

* fixup! feat(coap-server): add support for property meta operations

* test(coap-server): add basic tests for property meta operations
@JKRhb JKRhb mentioned this pull request Oct 9, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
binding-coap Issues related to coap protocol binding
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants