Remove nested violations #180

yevhenii-nadtochii · 2025-01-07T17:03:17Z

This PR does the following:

Removes usages of the deprecated (if_invalid) option. There's no error message for (validate) anymore.
Deprecates nested violations and removes its usages. We no longer create an umbrella violation for (validate).

I haven't introduced a nullable error message in BoolFieldOptionView for two reasons:

An empty error message can not be misused. (valiate) has its own CodeGenerator.
We would have to tackle with nullability down the Rule's framework, which is quite useless because Rules are Protobuf messages, where no string remains an empty string.

# Conflicts: # dependencies.md # pom.xml # version.gradle.kts

codecov · 2025-01-08T17:26:08Z

Codecov Report

Attention: Patch coverage is 33.33333% with 12 lines in your changes missing coverage. Please review.

Project coverage is 31.76%. Comparing base (6049654) to head (ab9112a).
Report is 23 commits behind head on master.

Additional details and impacted files

@@             Coverage Diff              @@
##             master     #180      +/-   ##
============================================
+ Coverage     31.53%   31.76%   +0.23%     
  Complexity      329      329              
============================================
  Files           142      142              
  Lines          2838     2814      -24     
  Branches        235      229       -6     
============================================
- Hits            895      894       -1     
+ Misses         1876     1855      -21     
+ Partials         67       65       -2

# Conflicts: # buildSrc/src/main/kotlin/io/spine/dependency/local/Base.kt # dependencies.md

yevhenii-nadtochii · 2025-01-09T09:45:28Z

@armiol @alexander-yevsyukov PTAL

armiol

@yevhenii-nadtochii please see my comments.

armiol · 2025-01-09T10:09:33Z

model/src/main/kotlin/io/spine/validation/ValidatedFieldView.kt

-        @External @Where(field = OPTION_NAME, equals = IF_INVALID) e: FieldOptionDiscovered
-    ) = super.onErrorMessage(e)
+    override fun extractErrorMessage(option: Option): String = error(
+        "Can not extract custom error message for `($VALIDATE)` option using `$option`. " +


armiol · 2025-01-09T10:12:02Z

proto/context/src/main/proto/spine/validation/field_views.proto

@@ -30,11 +30,8 @@ message ValidatedField {

    FieldId id = 1;

-    // Message appearing when this field is not valid.
-    string error_message = 2;


Given this is a part of the public API, I would deprecate the field first.

armiol · 2025-01-09T10:13:25Z

java-tests/validating/src/test/kotlin/io/spine/test/options/ValidateConstraintTest.kt

-        val violations = error.get().constraintViolationList
-
-        // We should have 2 violations instead of 1.
+        // We should have 3 violations instead of 2.


But below the code says violations.size shouldBe 2. Who's right?

armiol · 2025-01-09T10:15:32Z

model/src/main/kotlin/io/spine/validation/ValidatedFieldView.kt

+ * A view of a field that is marked with `(validate)`.
+ *
+ * Note: this option [does not have][NO_ERROR_MESSAGE] an error message. It triggers
+ * in-depth validation and, in case of errors, propagates only the initial cause.


"...propagates only the first one occurred."

armiol · 2025-01-09T10:16:34Z

model/src/main/kotlin/io/spine/validation/ValidatedFieldView.kt

-    ) = super.onErrorMessage(e)
+    override fun extractErrorMessage(option: Option): String = error(
+        "Can not extract custom error message for `($VALIDATE)` option using `$option`. " +
+                "`($VALIDATE)` does not support custom error message."


"... does not support custom error messages". Alternatively, if left singular, you will need an article.

alexander-yevsyukov

Please see my comments.

alexander-yevsyukov · 2025-01-09T10:18:03Z

java-runtime/src/main/proto/spine/validate/validation_error.proto

@@ -104,5 +104,5 @@ message ConstraintViolation {
    // This field is not populated, if fields of the corresponding message type do not have
    // validation constraints, and simply non-default value was required in the parent message.
    //
-    repeated ConstraintViolation violation = 5;
+    repeated ConstraintViolation violation = 5 [deprecated = true];


Please document the deprecation.

alexander-yevsyukov · 2025-01-09T10:22:09Z

model/src/main/kotlin/io/spine/validation/SimpleRules.kt

@@ -46,14 +46,10 @@ public fun SimpleRule(
    field: FieldName,
    customFeature: Message,
    description: String,
-    errorMessage: String,
+    errorMessage: String = "",


Please document the case when the error message could be empty, and why this is the default case.

alexander-yevsyukov · 2025-01-09T10:24:06Z

model/src/main/kotlin/io/spine/validation/ValidatedFieldView.kt

    }
 }
+
+private const val NO_ERROR_MESSAGE = ""


Please move it into a companion object.

yevhenii-nadtochii · 2025-01-09T11:24:04Z

@armiol @alexander-yevsyukov PTAL

alexander-yevsyukov

LGTM

yevhenii-nadtochii added 2 commits January 7, 2025 18:02

Don't use nested violations

9a12cd8

Deprecate field with nested violations

3e478fe

yevhenii-nadtochii self-assigned this Jan 7, 2025

yevhenii-nadtochii added 4 commits January 8, 2025 17:38

Take the latest base

8427747

Remove usages of if_invalid

8a983fd

Bump the version -> 2.0.0-SNAPSHOT.186

8da107f

Merge branch 'master' into remove-nested-violations

b557134

# Conflicts: # dependencies.md # pom.xml # version.gradle.kts

yevhenii-nadtochii added 10 commits January 9, 2025 09:28

Introduce constants

58e99b1

Remove no longer valid test case

d558857

Fix a comment

4b2b6fa

Use Kotlin API

5c092b9

Update docs

7c68a28

Fix code block generation

86a399f

Remove no longer used subscriber

740a276

Remove unused property from the projection state

8f9ebf7

Merge branch 'refs/heads/master' into remove-nested-violations

d9f2a7e

# Conflicts: # buildSrc/src/main/kotlin/io/spine/dependency/local/Base.kt # dependencies.md

Bump the version -> 2.0.0-SNAPSHOT.187

e7b583d

yevhenii-nadtochii marked this pull request as ready for review January 9, 2025 09:38

yevhenii-nadtochii requested review from alexander-yevsyukov and armiol January 9, 2025 09:45

armiol requested changes Jan 9, 2025

View reviewed changes

alexander-yevsyukov requested changes Jan 9, 2025

View reviewed changes

yevhenii-nadtochii added 6 commits January 9, 2025 11:41

Proofread the error message

1b87d09

Proofread docs

8a93809

Document deprecations

6b54db8

Move the constant to a companion object

5196807

Document an optional errorMessage

d205ea9

Remove the default value for SimpleRule() pseudo-constructor

ab9112a

yevhenii-nadtochii requested review from alexander-yevsyukov and armiol January 9, 2025 11:24

alexander-yevsyukov approved these changes Jan 9, 2025

View reviewed changes

armiol approved these changes Jan 9, 2025

View reviewed changes

yevhenii-nadtochii merged commit d681c35 into master Jan 10, 2025
7 checks passed

yevhenii-nadtochii deleted the remove-nested-violations branch January 10, 2025 11:06

yevhenii-nadtochii mentioned this pull request Mar 12, 2025

Refine structure of ConstraintViolation #177

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove nested violations #180

Remove nested violations #180

yevhenii-nadtochii commented Jan 7, 2025 •

edited

Loading

codecov bot commented Jan 8, 2025 •

edited

Loading

yevhenii-nadtochii commented Jan 9, 2025

armiol left a comment

armiol Jan 9, 2025

armiol Jan 9, 2025

armiol Jan 9, 2025

armiol Jan 9, 2025

armiol Jan 9, 2025

alexander-yevsyukov left a comment

alexander-yevsyukov Jan 9, 2025

alexander-yevsyukov Jan 9, 2025

alexander-yevsyukov Jan 9, 2025

yevhenii-nadtochii commented Jan 9, 2025

alexander-yevsyukov left a comment

Remove nested violations #180

Remove nested violations #180

Conversation

yevhenii-nadtochii commented Jan 7, 2025 • edited Loading

codecov bot commented Jan 8, 2025 • edited Loading

Codecov Report

yevhenii-nadtochii commented Jan 9, 2025

armiol left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alexander-yevsyukov left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yevhenii-nadtochii commented Jan 9, 2025

alexander-yevsyukov left a comment

Choose a reason for hiding this comment

yevhenii-nadtochii commented Jan 7, 2025 •

edited

Loading

codecov bot commented Jan 8, 2025 •

edited

Loading