SpineEventEngine / validation

Apache License 2.0
3 stars 0 forks source link

Support `(set_once)` in validation codegen #138

Closed yevhenii-nadtochii closed 1 week ago

yevhenii-nadtochii commented 3 weeks ago

This PR extends coverage of validation codegen to support (set_once) constraint.

The option is enforced by checking (during a field value assignment) that the current value is either default for the field type OR it is the same with the assigned one. Otherwise, the validation exception is thrown right away.

When one or more fields with the message are marked with the option, the following groups of methods are modified to add the precondition checks:

  1. All set...() methods for the marked field.
  2. All merge...() methods, except for AbstractMessage.mergeFrom(final Message other). In merge methods, only parts relevant to the marked fields are affected.

AbstractMessage.mergeFrom(final Message other)

This method is not explicitly supported, though it might be workable.

Supported field types

All primitives, enums and messages are supported.

An attempt to mark repeated or map field leads to an exception.

yevhenii-nadtochii commented 3 weeks ago

@armiol @alexander-yevsyukov

We should settle down on the option specification before proceeding with this PR. I've examined the generated code, and the main disappointment is that Protobuf v3 doesn't store explicit information about whether the field was set (except for optional fields). Our implementation checks that the current field value is default for the field type, only then allowing a new one to set.

In general, Protobuf builders provide three paths to modify the message.

set...():

  1. Direct field setter like setName(String name).
  2. Alternative setters like setName(ByteString name) for string and setMyEnumValue(int value) for enums.
  3. Direct field setter that accepts a builder instead of a message (only for message-based fields) like setStudent(StudentBuilder builder).
  4. Reflective setter, which sets a field by its descriptor setField(FieldDescriptor, Object).

merge...():

  1. Field merging (only for message-based fields) like mergeName(Name value).
  2. Message merging mergeFrom(Student other).
  3. Bytes merging mergeFrom(byte[] data).

clear...():

  1. Clear a specific field clearName().
  2. Clear the whole message clear().

The questions are the following:

  1. Should we allow a custom error message for this option?
  2. Which field types are supported? - I suggest messages, enumerations and all primitives excluding repeated fields and maps. bytes arrays are also allowed.
  3. Which points of message mutation does the option cover? – All setters for sure. But should we cover merging and clearing? The current implementation implicitly covers because it just compares a field value having two message instances. The problem is that for many field types, the merging is based on setters. We have to overcome this. And clearing allows overriding a field value, for which (set_once) is specified.
  4. Should we allow the field to be set to the current value? – Looks logical if we cover merge-based mutations. Looks safe for setters as well.
  5. Should we prohibit clearing of a specific field if it is marked with the option? What about the builder clear() then? – I suggest considering merge and clear service methods, and don't touch them at all. Only fix the fact that for some types, merge redirect to setters. We can introduce private unsafe setters for that, for example.
alexander-yevsyukov commented 3 weeks ago
  1. Should we allow a custom error message for this option?

This is highly desirable. People creating their domain language would want to explain why a field can be set only once. These cases are usually non-trivial.

alexander-yevsyukov commented 3 weeks ago
  1. Which field types are supported? - I suggest messages, enumerations and all primitives excluding repeated fields and maps. bytes arrays are also allowed.

I concur.

alexander-yevsyukov commented 3 weeks ago
  1. Which points of message mutation does the option cover? – All setters for sure. But should we cover merging and clearing?

Merging — it is desirable.

See on clearing below.

... And clearing allows overriding a field value, for which (set_once) is specified.

The (set_once) option largely protects from setting the value via a setter. If the developer explicitly calls clear() it corresponds to starting all over on this message. Something happened in the domain, which cased the code to clear the message. To me it close to having newBuilder(). I suggest viewing clear() this way.

alexander-yevsyukov commented 3 weeks ago
  1. Should we allow the field to be set to the current value?

I think this way it's understandable and safe. We should allow this, provided this is documented in the option documentation.

alexander-yevsyukov commented 3 weeks ago
  1. Should we prohibit clearing of a specific field if it is marked with the option?

Let's allow clearing such fields. Please also see my earlier comment regarding the clear operation.

What about the builder clear() then?

We should allow this. See my earlier comment on the "weight" of the clear operation.

@armiol, FYI.

codecov[bot] commented 1 week ago

Codecov Report

Attention: Patch coverage is 0.45045% with 221 lines in your changes missing coverage. Please review.

Project coverage is 34.21%. Comparing base (52a2648) to head (6994ce4). Report is 97 commits behind head on master.

Additional details and impacted files ```diff @@ Coverage Diff @@ ## master #138 +/- ## ============================================ - Coverage 37.50% 34.21% -3.29% + Complexity 362 360 -2 ============================================ Files 126 134 +8 Lines 2488 2709 +221 Branches 209 220 +11 ============================================ - Hits 933 927 -6 - Misses 1493 1719 +226 - Partials 62 63 +1 ```
yevhenii-nadtochii commented 1 week ago

@armiol @alexander-yevsyukov PTAL

yevhenii-nadtochii commented 1 week ago

@alexander-yevsyukov @armiol PTAL