Skip to content

nanocoap: add coap_iterate_uri_query()#20197

Merged
benpicco merged 2 commits intoRIOT-OS:masterfrom
benpicco:coap_iterate_uri_query
Jul 24, 2025
Merged

nanocoap: add coap_iterate_uri_query()#20197
benpicco merged 2 commits intoRIOT-OS:masterfrom
benpicco:coap_iterate_uri_query

Conversation

@benpicco
Copy link
Copy Markdown
Contributor

@benpicco benpicco commented Dec 19, 2023

Contribution description

This adds a convenience function to iterate over a packets's URI query options.
Options have the format key=value, but options that only consist of a key may also exist.
For consistency, in this case the value is set to "1".

Testing procedure

I extended the tests-nanocoap unit test with the new function.

Issues/PRs references

@benpicco benpicco requested review from bergzand, chrysn and maribu and removed request for PeterKietzmann, cgundogan, haukepetersen and miri64 December 19, 2023 14:21
@github-actions github-actions bot added Area: network Area: Networking Area: tests Area: tests and testing framework Area: CoAP Area: Constrained Application Protocol implementations Area: sys Area: System labels Dec 19, 2023
chrysn
chrysn reviewed Dec 19, 2023
View reviewed changes
@chrysn chrysn mentioned this pull request Dec 19, 2023
Merged
HendrikVE
HendrikVE reviewed Dec 19, 2023
View reviewed changes
chrysn
chrysn reviewed Dec 19, 2023
View reviewed changes
Copy link
Copy Markdown
Member

@chrysn chrysn left a comment •
edited
Loading

Choose a reason for hiding this comment

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

See added comments on the implementation.

I'm not sure this is the right abstraction level. For those who need all the options, a loop using coap_opt_get_opaque is straightforward enough to do where it is needed. For users who really want to parse this data, in most cases they already know the keys they are looking for (and may even opt to ignore keys they do not know, even though I do not condone that use of query parameters) -- and for those, I think something getopt-style would be more convenient. A la

int32_t value = 3600;
coap_get_uri_query_i32(pkt, "lt", *value);

which would return 0 on OK, or -ENOENT when not found, or something else if there are multiple occurrences, but is still usable in the absence of error handling.

@benpicco
Copy link
Copy Markdown
Contributor Author

benpicco commented Dec 19, 2023 •
edited
Loading

Expecting the user to iterate coap_opt_get_opaque() for this is not very user friendly.
While I like the idea of querying the desired key directly, it would involve iterating the packet multiple times if we are interested in multiple keys.

With this we can just call coap_iterate_uri_query() in a loop and check all they keys that fall out.

@Teufelchen1
Copy link
Copy Markdown
Contributor

Teufelchen1 commented Dec 19, 2023

Slightly off-topic:

Details Feels a bit odd that riot has a "uri parser" module but this PR adds a function with "uri" and "query" in its name that is not in that module nor does it use functions of that module. Don't get me wrong, I think this PR is fine in this regard - Just makes me wonder a bit.

@chrysn
Copy link
Copy Markdown
Member

chrysn commented Dec 19, 2023 via email

@chrysn
Copy link
Copy Markdown
Member

chrysn commented Dec 19, 2023

Slightly off-topic:

The long answer to this is in #13827, which could be a good base for a consistent interface for URIs and locations both on clients and servers, as well as references -- but that's still waiting for the spec to stabilize.

@benpicco
Copy link
Copy Markdown
Contributor Author

benpicco commented Dec 19, 2023 •
edited
Loading

We'd still have to copy the value with coap_get_uri_query(coap_pkt_t *pkt, const char *key, char *value, size_t value_len_max);, but just that - maybe that's more ergonomic indeed 🤔

(I don't want a separate function for very possible type of value, as value is always a string the function should return a string)

chrysn
chrysn reviewed Dec 19, 2023
View reviewed changes
@Teufelchen1
Copy link
Copy Markdown
Contributor

Teufelchen1 commented Dec 19, 2023

Could you add two tests for the cases of returning -E2BIG?

In general, the unit tests lack in quality but a refactor is out of scope for this PR.

@benpicco benpicco force-pushed the coap_iterate_uri_query branch from 8b5bbae to cbad490 Compare December 19, 2023 15:23
@benpicco benpicco mentioned this pull request Jan 18, 2024
Merged
@benpicco
Copy link
Copy Markdown
Contributor Author

benpicco commented Jan 18, 2024

I think even with #20273 this PR is still useful.
While coap_find_uri_query() is convenient when you only expect a very limited number of URI query options, it gets unwieldy quickly if your API allows for many option keys so instead of iterating all possible keys, you'd only iterate the keys present with coap_iterate_uri_query().

Teufelchen1
Teufelchen1 requested changes Mar 12, 2024
View reviewed changes
Copy link
Copy Markdown
Contributor

@Teufelchen1 Teufelchen1 left a comment

Choose a reason for hiding this comment

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

Some Nits :)

@Teufelchen1
Copy link
Copy Markdown
Contributor

Teufelchen1 commented Mar 18, 2024

Squash'n rebase?

@benpicco benpicco force-pushed the coap_iterate_uri_query branch from fe4b00e to 418b668 Compare April 29, 2025 17:38
@benpicco benpicco added the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Apr 29, 2025
@riot-ci
Copy link
Copy Markdown

riot-ci commented Apr 29, 2025 •
edited
Loading

Murdock results

✔️ PASSED

09fb152 tests/unittests: extend tests with coap_iterate_uri_query()

Success Failures Total Runtime
10522 0 10524 10m:21s

Artifacts

@crasbe crasbe added the Type: new feature The issue requests / The PR implemements a new feature for RIOT label Apr 29, 2025
@benpicco
Copy link
Copy Markdown
Contributor Author

benpicco commented Jul 23, 2025

@Teufelchen1 ping 😉

Teufelchen1
Teufelchen1 reviewed Jul 24, 2025
View reviewed changes
@Teufelchen1
Copy link
Copy Markdown
Contributor

Teufelchen1 commented Jul 24, 2025

Squash please!

@benpicco benpicco force-pushed the coap_iterate_uri_query branch from 6acdd0c to 09fb152 Compare July 24, 2025 11:18
@benpicco benpicco added this pull request to the merge queue Jul 24, 2025
Merged via the queue into RIOT-OS:master with commit 4f9f755 Jul 24, 2025
25 checks passed
@benpicco benpicco deleted the coap_iterate_uri_query branch July 25, 2025 10:27
@benpicco benpicco added this to the Release 2025.10 milestone Dec 3, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@chrysn chrysn chrysn left review comments

@Teufelchen1 Teufelchen1 Teufelchen1 approved these changes

@maribu maribu Awaiting requested review from maribu

@bergzand bergzand Awaiting requested review from bergzand

+1 more reviewer

@HendrikVE HendrikVE HendrikVE left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

Area: CoAP Area: Constrained Application Protocol implementations Area: network Area: Networking Area: sys Area: System Area: tests Area: tests and testing framework CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR Type: new feature The issue requests / The PR implemements a new feature for RIOT

Projects

None yet

Milestone

Release 2025.10

Development

Successfully merging this pull request may close these issues.

6 participants

@benpicco @Teufelchen1 @chrysn @riot-ci @HendrikVE @crasbe