Need setters to do validation when setting message fields

[maximecb]

There's an issue where some fields can't accept some values. For example, an unsigned integer can't be set to a negative value.

Right now, the code to encode unsigned integers will just go into an infinite loop when trying to encode if we set an unsigned integer field to -1 for example. This is clearly not desirable or expected.

IMO, these kinds of validation errors should be caught as early as possible, ideally when field values are set, rather than when messages are encoded, and so we need some validation logic inside setter methods for message fields.

This is technically to protect protoboeuf users from themselves, so maybe not as high priority as other things, but it needs to happen eventually, and I think catching negative values or values out of range is probably the most important kind of validation that we need to do.

[services-db[bot]]

You have created a Bug or Security issue without a Severity label of the format SEV-X.

If your service has components, you should also add a Component label of the format Component: X. For example: Component: payments

It is important for those to be correctly labeled and categorized so we have better visibility over our development and security health practices and make sure we are prioritizing the right investments.

The untriaged label has been added. Please remove it once the team has set the correct severity for this issue.

Learn more in this Vault Page.

[tenderworks]

IMO, these kinds of validation errors should be caught as early as possible, ideally when field values are set, rather than when messages are encoded, and so we need some validation logic inside setter methods for message fields.

I think we should only do validation (if any) in the encoder. I agree it's nice to get the exception at the call site, but this is the type of type checking I want to get away from. For example in protobuf, you can specify homogeneous arrays so we'd have to provide custom collections that implement this type of validation (which is what Google's implementation does). I want users of ProtoBoeuf to interact with the library as they would any other Ruby object by using Ruby arrays, Ruby hashes, attr_reader / writer, etc.

We can still be helpful when validating in the encoder by telling the user what field was invalid (for example). Additionally, I think we should (eventually) generate Sorbet type signatures so that we won't need most validation (besides signed numbers, probably).

[maximecb]

Yes we should only do validation in the encoder.

We kind of have no choice to do some validation because some inputs are invalid (e.g. number out of range). We either do the validation in setters (probably better, quicker developer feedback), or we do it while encoding these values, which would make the code we generate more messy.

I want users of ProtoBoeuf to interact with the library as they would any other Ruby object by using Ruby arrays, Ruby hashes, attr_reader / writer, etc.

I agree that a more Ruby-native interface is nicer. At the moment mostly concerned with numbers out of range and such, or someone supplying a floating-point value for an int field, etc.

[TobiasBales]

The wrong type supplied could probably be mitigated by using sorbet (I just opened a PR for generated sorbet types from the definition).

I would see about adding range validation next, question would be - encoder or setter?