Properly validate `Oneof` fields #240

dmytro-grankin · 2018-11-02T10:20:54Z

This PR fixes #154.

Now Oneof fields are considered in a special way and only the actually set field is validated. For example, if a Oneof contains 3 fields, the only one is validated. The work is done by a newly introduced OneofValidator.

The important note is that if none of a Oneof fields are set, a declaration is considered invalid.

So, after the change, it is possible to create a message where every Oneof field is required (see the EveryRequired below). And the message is considered valid if the first or second field is set.

message EveryRequired {
    oneof fields {
        string first = 1 [(required) = true];
        string second = 2 [(required) = true];
    }
}

Also, the config was updated.

# Conflicts: # base/src/main/java/io/spine/validate/MessageValidator.java # base/src/test/java/io/spine/validate/MessageValidatorTest.java

codecov · 2018-11-02T10:30:03Z

Codecov Report

Merging #240 into master will increase coverage by 0.16%.
The diff coverage is 100%.

@@             Coverage Diff              @@
##             master     #240      +/-   ##
============================================
+ Coverage     66.22%   66.38%   +0.16%     
  Complexity      583      583              
============================================
  Files           309      310       +1     
  Lines          8118     8158      +40     
  Branches        560      561       +1     
============================================
+ Hits           5376     5416      +40     
  Misses         2625     2625              
  Partials        117      117

dmytro-grankin · 2018-11-02T18:12:22Z

@armiol PTAL.

alexander-yevsyukov · 2018-11-03T11:15:37Z

base/src/main/java/io/spine/validate/OneOfValidator.java

+                                     .toBuilder()
+                                     .addFieldName(oneOf.getName())
+                                     .build();
+        ConstraintViolation requiredFieldNotFound =


Can we have the type on the previous line (preparing for Java 11 var syntax)?

Suggested change

ConstraintViolation requiredFieldNotFound =

ConstraintViolation requiredFieldNotFound = ConstraintViolation

.newBuilder()

alexander-yevsyukov · 2018-11-03T11:16:22Z

base/src/test/java/io/spine/validate/MessageValidatorTest.java

+        @Test
+        @DisplayName("valid if a required field is set to a non-default value")
+        void validIfRequireFieldIsNotDefault() {
+            EveryRequired requiredIsNotDefault = EveryRequired.newBuilder()


Please have newBuilder() here and below in new lines.

armiol

Please see my comments.

armiol · 2018-11-03T16:08:10Z

base/src/main/java/io/spine/validate/OneOfValidator.java

+ * <p>Only a single field from a {@code OneOf} is validated — the field that is actually set.
+ * If none of fields is set, a constraint violation is created.
+ *
+ * @see <a href="https://developers.google.com/protocol-buffers/docs/proto3#oneof">One Of documentation</a>


As long as Protobuf documentation references the language element as Oneof, let's do the same in our code, docs and PR description. I.e. not OneOf or One Of.

armiol · 2018-11-03T16:10:48Z

base/src/test/proto/spine/test/validate/oneof.proto

+message OneRequired {
+    oneof fields {
+        string required_val = 1 [(required) = true];
+        string non_required = 2 [(required) = false];


It should probably be optional.

armiol · 2018-11-03T16:11:00Z

base/src/test/proto/spine/test/validate/oneof.proto

+    }
+}
+
+message EveryNotRequired {


And this one EveryOptional.

armiol · 2018-11-03T16:12:21Z

base/src/main/java/io/spine/validate/MessageValue.java

+    /**
+     * Obtains field values of the message.
+     *
+     * <p>Values of {@code OneOf} fields are filtered and not returned.


...are filtered out...

armiol · 2018-11-03T16:13:35Z

base/src/main/java/io/spine/validate/MessageValue.java

+     *
+     * @param oneOf
+     *         the {@code OneOf} descriptor
+     * @return a value of the populated field or {@code Optional.empty()} if the message


It seems to me that if the message does not contain the field, which description was passed to the method, we should throw an invalid argument exception.

dmytro-grankin · 2018-11-05T09:36:01Z

@alexander-yevsyukov PTAL.

dmytro-grankin · 2018-12-10T12:36:33Z

@armiol PTAL.

dmytro-grankin added 8 commits October 29, 2018 11:42

Add tests and skeleton of OneOf validation

061ff95

Merge branch 'refactor-field-validators' into required-oneof-fields

1da813a

# Conflicts: # base/src/main/java/io/spine/validate/MessageValidator.java # base/src/test/java/io/spine/validate/MessageValidatorTest.java

Use MessageValue inside OneOfValidator

fa07bbc

Validate OneOf fields only by OneOfValidator

1f15812

Set the path of non-set OneOf field

018a3a2

Add class-level Javadoc for OneOfValidator

88f8a29

Merge branch 'refactor-field-validators' into required-oneof-fields

58213b7

Merge branch 'master' into required-oneof-fields

c8f67f6

dmytro-grankin self-assigned this Nov 2, 2018

Update config

05cb1c9

dmytro-grankin requested a review from armiol November 2, 2018 18:12

alexander-yevsyukov reviewed Nov 3, 2018

View reviewed changes

armiol reviewed Nov 3, 2018

View reviewed changes

Fix line wrapping

9a50bd0

dmytro-grankin changed the title ~~Properly validate OneOf fields~~ Properly validate Oneof fields Nov 5, 2018

dmytro-grankin added 4 commits November 5, 2018 10:41

Use Oneof instead of OneOf

6e63f05

Improve naming of test declarations

9987c0e

Throw IllegalArgumentException if a message doesn't declare oneof

ee8042e

Fix line wrapping

a34050c

Merge branch 'master' into required-oneof-fields

9d9b0b8

dmytro-grankin added 2 commits December 11, 2018 19:20

Merge branch 'master' into required-oneof-fields

ed01dd1

Merge branch 'master' into required-oneof-fields

6bb1c99

armiol approved these changes Dec 12, 2018

View reviewed changes

Merge branch 'master' into required-oneof-fields

eff89c1

dmytro-grankin merged commit 844985e into master Dec 12, 2018

dmytro-grankin deleted the required-oneof-fields branch December 12, 2018 15:06

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Properly validate `Oneof` fields #240

Properly validate `Oneof` fields #240

Uh oh!

dmytro-grankin commented Nov 2, 2018 •

edited

Loading

Uh oh!

codecov bot commented Nov 2, 2018 •

edited

Loading

Uh oh!

dmytro-grankin commented Nov 2, 2018

Uh oh!

alexander-yevsyukov Nov 3, 2018

Uh oh!

alexander-yevsyukov Nov 3, 2018

Uh oh!

armiol left a comment

Uh oh!

armiol Nov 3, 2018

Uh oh!

armiol Nov 3, 2018

Uh oh!

armiol Nov 3, 2018

Uh oh!

armiol Nov 3, 2018

Uh oh!

armiol Nov 3, 2018

Uh oh!

dmytro-grankin commented Nov 5, 2018

Uh oh!

dmytro-grankin commented Dec 10, 2018

Uh oh!

Uh oh!

	ConstraintViolation requiredFieldNotFound =
	ConstraintViolation requiredFieldNotFound = ConstraintViolation
	.newBuilder()

Properly validate Oneof fields #240

Properly validate Oneof fields #240

Uh oh!

Conversation

dmytro-grankin commented Nov 2, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov bot commented Nov 2, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

dmytro-grankin commented Nov 2, 2018

Uh oh!

alexander-yevsyukov Nov 3, 2018

Choose a reason for hiding this comment

Uh oh!

alexander-yevsyukov Nov 3, 2018

Choose a reason for hiding this comment

Uh oh!

armiol left a comment

Choose a reason for hiding this comment

Uh oh!

armiol Nov 3, 2018

Choose a reason for hiding this comment

Uh oh!

armiol Nov 3, 2018

Choose a reason for hiding this comment

Uh oh!

armiol Nov 3, 2018

Choose a reason for hiding this comment

Uh oh!

armiol Nov 3, 2018

Choose a reason for hiding this comment

Uh oh!

armiol Nov 3, 2018

Choose a reason for hiding this comment

Uh oh!

dmytro-grankin commented Nov 5, 2018

Uh oh!

dmytro-grankin commented Dec 10, 2018

Uh oh!

Uh oh!

Properly validate `Oneof` fields #240

Properly validate `Oneof` fields #240

dmytro-grankin commented Nov 2, 2018 •

edited

Loading

codecov bot commented Nov 2, 2018 •

edited

Loading