search

eclipse-uprotocol / up-cpp

uProtocol Language Specific Library for C++
Apache License 2.0
18 stars 25 forks source link

MicroUriSerializer check has incorrect logic #102

Closed ALPayne18 closed 5 months ago

ALPayne18 commented 5 months ago

When a Micro URI is validated, it assumes that the entity.id() must be greater than 0.   https://github.com/eclipse-uprotocol/up-cpp/blob/494b3313b55aaefe5b8b0376dd761b75fa96a78a/include/up-cpp/uri/serializer/MicroUriSerializer.h#L193C1-L195C6

However, this is not the case. The core uSubscription uService has an id == 0: 

// Subscription Service Interface definition
service uSubscription {
  option (name) = "core.usubscription"; // Service name
  option (version_major) = 3;
  option (version_minor) = 0;
  option (id) = 0;

This bug causes any URI for the uSubscription service to fail to serialize.

PLeVasseur commented 5 months ago

Hi @ALPayne18 --

This does look to be a legit bug.

I can find nowhere in the Micro UUri spec mention of restricting the UEntity id to be non-zero.

Further review of up-rust and up-java also shows no such restriction in-place.

@gregmedd -- how'd we like to handle this?

gregmedd commented 5 months ago

The spec explicitly calls out an ID of 0 as being for uSubscription, so I agree it needs to be fixed. Is @debruce or @billpittman available to grab this issue and put out a PR with a fix?

gregmedd commented 5 months ago

This has been resolved with #104. Do we need to tag a release, or do we have other bugfixes?

billpittman commented 5 months ago

FWIW, this has been merged and theoretically fixed, but I can't assign it to myself, nor close it. However it should be at least closed.