Increase strictness / correctness of URI micro form validation

[PLeVasseur]

Added helper functions on UResource and UEntity

During serialization of UURI to micro form, perform checks on

1. existence of UResource/UEntity
2. existence of id
3. id not overflowing allotted bits
4. existence of major version
5. major version not overflowing allotted bits

Also updated is_micro_form() to account for being false in the case that the ids are outside of expected range of 16 bits or UEntity major version it outside of expected range of 8 bits.

Note: Based on an issue I raised over here.

[stevenhartley]

@AnotherDaniel & @sophokles73 please comment on this PR

[sophokles73]

I did not go through all of your changes. In general, I do not like the overall idea of having all the serialization and validation logic in different modules. FMPOV all of this functionality should be in the corresponding structs (thus improving cohesion) and the constraints on the fields should then be checked during creation (or deserialization) and serialization of the entities.

[PLeVasseur]

I did not go through all of your changes. In general, I do not like the overall idea of having all the serialization and validation logic in different modules. FMPOV all of this functionality should be in the corresponding structs (thus improving cohesion) and the constraints on the fields should then be checked during creation (or deserialization) and serialization of the entities.

First off, thanks for the review! My first Rust code review 😄 Gonna have to get this framed.

Lemme try to break the review down to make sure I'm groking it.

In general, I do not like the overall idea of having all the serialization and validation logic in different modules. FMPOV all of this functionality should be in the corresponding structs (thus improving cohesion)

Let me try to reword it to see if I get it:

UriValidator::validate_micro_form() will continue to exist (since it has to get called by uP-L1 and uP-L2 APIs to validate the UURIs prior to their use), however...

its form will change to from the currently, quite large function to a much smaller one along the lines of...

    #[allow(clippy::missing_panics_doc)]
    pub fn validate_micro_form(uri: &UUri) -> Result<(), ValidationError> {
        if Self::is_empty(uri) {
            Err(ValidationError::new("URI is empty"))?;
        }

       if !UEntity::validate_micro_form(uri) {
            Err(ValidationError::new("UEntity invalid for micro form"))?;
       }

       if !UAuthority::validate_micro_form(uri) {
            Err(ValidationError::new("UAuthority invalid for micro form"))?;
       }

       if !UResource::validate_micro_form(uri) {
            Err(ValidationError::new("UResource invalid for micro form"))?;
       }

       Ok(())
    }

Keep in mind I'm just riffing here, so I'd probably have the validate_micro_form() function on each of UEntity, UResource, and UAuthority return a ValidationError so that I can bubble up / concatenate the reason for the error like I mentioned wanting to do over in this comment.

and the constraints on the fields should then be checked during creation (or deserialization) and serialization of the entities.

Agree on the serialization bit -- the MicroUriSerializer::serialize() function would call UriValidator::validate_micro_form(), as would any implementer of uTransport if e.g. calling send().

As for on creation, I honestly went back and forth in my mind about it. Because the structs are exposed thru the .proto files with the fields as pub, they could be arbitrarily constructed in any way by a user of the SDK which would not pass validation. There's really no way with our current APIs to prevent that from happening (I have thoughts on this for another day that this is not great, but this is already getting long).

I do like the idea of having builders which can help create the individual pieces in a known good fashion. I began work on, but put down for now, some work on that over in this branch. If you think the idea has some legs, I'll work on it a bit more.

TL;DR: Your suggestion is to move the logic for validation of whether the UURI satisfies micro form for a particular struct (e.g. UEntity), into that struct's impl and clean up validate_micro_form() to be the shorter version I wrote above.

[AnotherDaniel]

I do like the idea of having builders which can help create the individual pieces in a known good fashion. I began work on, but put down for now, some work on that over in this branch. If you think the idea has some legs, I'll work on it a bit more.

This ties in with this discussion, about reintroducing Builders for the core .proto objects. I'd be for that... and then the protoc-generated rust code could be made non-public. Thus we would hide the dependency to that code, and potential vagaries of protoc etc from the SDK users.
Thoughts?

[PLeVasseur]

I like that idea a lot!

@AnotherDaniel, @sophokles73 -- where would make sense to go from here? Close this PR and pick back up my other PR with this kinda functionality also included?

Or change this PR to kinda do what Kai was suggesting and then tackle the builders in a new PR?

[sophokles73]

@PLeVasseur FMPOV we should start with moving the checks into the corresponding structs.
Then, in an additional PR, you could start adding builders ...

[PLeVasseur]

@sophokles73 -- could you take a look again to see I addressed your concern? I broke the validation logic for micro URI form up into the impls of the constituent members of a UUri.

[PLeVasseur]

@sophokles73 -- I rebased & updated to bail out of the validate_micro_form() functions on first fail of validation.

Could you take a look again?

[sophokles73]

You got the idea 👍
I made some suggestions for improvement. WDYT?

[PLeVasseur]

You got the idea 👍 I made some suggestions for improvement. WDYT?

Alrighty, I updated based on your suggestions. Can you take another look?

[PLeVasseur]

Hey folks -- sorry I missed that the up-core-api change got merged fixing the ambiguity around id/ip. I'll have a go at updating this to unblock #28

cc @sophokles73 @AnotherDaniel

[PLeVasseur]

Hey folks -- sorry I missed that the up-core-api change got merged fixing the ambiguity around id/ip. I'll have a go at updating this to unblock #28

Even better, @AnotherDaniel's PR got in first! Makes life easy.

Could you take a look and see if I hit the points we discussed the other day @sophokles73?

[sophokles73]

maybe you can add an EOL here?

[sophokles73]

I'd rather remove these literal comparisons as they are hard to maintain. In particular, when not using constants for the messages ...

[PLeVasseur]

Yeah, I could go through this file and remove these.

To a degree I was following along with the rest of the unit tests.

You want me to remove them all from this file?

[sophokles73]

FMPOV we (i.e. you ;-)) should

[PLeVasseur]

Okay, went ahead and did that, could you take another look?

[sophokles73]

LGTM

[sophokles73]

@stevenhartley can you approve the workflow run so that we can see that the changes pass all checks? And then merge ...