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

nanocoap_sock: always use coap_opt_put_uri_pathquery() #20245

Merged
merged 2 commits into from
Feb 19, 2024

Conversation

benpicco
Copy link
Contributor

@benpicco benpicco commented Jan 11, 2024

Contribution description

This is pretty subtle: coap_opt_put_uri_pathquery() will detect the presense of a query parameter, coap_opt_put_uri_path() will not.

That means that when you request path/to/resource?option=1 with coap_opt_put_uri_path() you get the path resource?option=1 whereas with coap_opt_put_uri_pathquery() you get the path resource with query parameter option=1.

We really want the latter.

Testing procedure

Issues/PRs references

@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 Jan 11, 2024
@github-actions github-actions bot added Area: network Area: Networking Area: CoAP Area: Constrained Application Protocol implementations Area: sys Area: System labels Jan 11, 2024
Copy link
Member

@chrysn chrysn left a comment

Choose a reason for hiding this comment

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

LGTM in the sense that it is probably what people need and looks correct from an implementation PoV.

The docs will need updating in all the functions like nanocoap_sock_get that previously speak of a "remote path" to now say "remote path and query".

And it is an API change, moreover, one that is not trivially raising compiler warnings. Given that it's likely reflecting any user's intention, it should suffice to add the right labels to make sure it gets a prominent note in the change log.

sys/net/application_layer/nanocoap/sock.c Outdated Show resolved Hide resolved
sys/net/application_layer/nanocoap/sock.c Outdated Show resolved Hide resolved
@riot-ci
Copy link

riot-ci commented Jan 11, 2024

Murdock results

✔️ PASSED

eb76b8e nanocoap: allow lastonum=NULL in coap_opt_put_uri_pathquery()

Success Failures Total Runtime
8101 0 8101 10m:26s

Artifacts

@chrysn chrysn added the Process: API change Integration Process: PR contains or issue proposes an API change. Should be handled with care. label Jan 11, 2024
@benpicco
Copy link
Contributor Author

To me it feels more like a bug fix than an API change - when I added the functions I was not aware of the difference between coap_opt_put_uri_pathquery() and coap_opt_put_uri_path() (or that there are in fact two different functions) 😅

@benpicco benpicco added the Type: bug The issue reports a bug / The PR fixes a bug (including spelling errors) label Jan 18, 2024
@benpicco
Copy link
Contributor Author

Is there anything I can do to move this forward?

@maribu maribu added this pull request to the merge queue Feb 19, 2024
Merged via the queue into RIOT-OS:master with commit 609ad44 Feb 19, 2024
27 checks passed
@benpicco benpicco deleted the nanocoap_sock-pathquery branch February 19, 2024 21:59
@benpicco
Copy link
Contributor Author

Thank you!

@MrKevinWeiss MrKevinWeiss added this to the Release 2024.04 milestone Apr 30, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area: CoAP Area: Constrained Application Protocol implementations Area: network Area: Networking Area: sys Area: System CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR Process: API change Integration Process: PR contains or issue proposes an API change. Should be handled with care. Type: bug The issue reports a bug / The PR fixes a bug (including spelling errors)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants