shamblett / coap

A Coap package for dart
Other
16 stars 13 forks source link

fix: allow empty URI path segments in requests #116

Closed JKRhb closed 1 year ago

JKRhb commented 1 year ago

Interacting with other CoAP libraries, we noticed in my project that URI paths seem to not be converted into options correctly. In particular, it seems as if empty URI path segments are supposed to be converted into empty URI-path options. Currently, these empty segments are removed from the URI path completely.

This PR adjusts the corresponding creation method accordingly, which should also make the library's behavior more consistent with section 6.4 of RFC 7252.

JosefWN commented 1 year ago

Should we do the same for uriLocationPath?

JKRhb commented 1 year ago

Should we do the same for uriLocationPath?

Good point!

I adjusted the code, I am not entirely sure if this is the correct way to handle this option, though. Let me know if you have any suggestions :)

JosefWN commented 1 year ago

We could sneak a peak at Californium :)

https://github.com/eclipse/californium/blob/f4680981c68ec48b2ecb6dc572ec5bd43300a208/californium-core/src/main/java/org/eclipse/californium/core/coap/OptionSet.java#L659

Seems similar to our approach, but if we want to make the code a tiny bit shorter:

var trimmedPath = fullPath;
if (fullPath.startsWith('/')) {
  trimmedPath = fullPath.substring(1);
}
JKRhb commented 1 year ago

Good catch, could clean up the locationPath setter a lot :)

JosefWN commented 1 year ago

Good catch, could clean up the locationPath setter a lot :)

I think we can do the same for uriPath, but it's just nitpicks anyway. The PR LGTM!

JKRhb commented 1 year ago

I think we can do the same for uriPath, but it's just nitpicks anyway.

Cleaned up that method as well now :)

The PR LGTM!

🎉

JosefWN commented 1 year ago

Great work on RFC compliance in general, and sorry for the sloppy URI/query validation I added in my previous PR. Should have checked the RFC, I've been a bit stressed and busy with other things lately.