Refactor `JavaValidationRenderer` by yevhenii-nadtochii · Pull Request #185 · SpineEventEngine/validation

yevhenii-nadtochii · 2025-01-16T14:08:45Z

This PR refactors JavaValidationRenderer and suggests renaming of MessageValidation → CompilationMessage. I need this change to proceed with the migration to PSI and simultaneous support of rules and standalone generators (as we complete the migration to the latter).

Changes to JavaValidationRenderer:

Revises the class top-level documentation. It was outdated.
Decreases the number of intermediate and file-level methods in the file.
The top-level documentation is directly mapped to the body of render() method now. A significant part of this method is local comments, describing why all modifications of a message file cannot be done under a single forEach. I describe this inconsistency, but leave it "as is" because I'm going to migrate to PSI in the upcoming PRs.
I removed the protected modifier from render() because it is unnecessary with override, and makes the entry point a bit harder to locate.

Other changes:

I have removed Validations collection, we can easily go without this class. We already have many classes with "Validation" in their names. Takes time to differentiate their purposes.
Moved declaration of FieldId up in field_views.proto file.
Renamed MessageValidation to CompilationMessage.
3.1. This projection stores views for each message being compiled.
3.2. Having one or more constraints is optional. We always add validate() to all messages (and related stuff), which go through the library no matter if they have constraints or not.
3.3. When we complete migration from Rule, this class will disappear completely. Each generator will query only a view of interest, like RequiredFieldView or DistinctFieldView.
3.4. Its usages talked more about a compilationMessage, and less as of a set of constraints.
3.5. This eliminates another class with "Validation" in its name.

Unrelated changes:

Renamed a local variable in SetOnceValidationRenderer. Both options are good enough, but the new one looks a bit better to me. And JavaValidationRenderer also uses allCompilationMessages.
Took the latest ProtoData.

yevhenii-nadtochii · 2025-01-16T16:29:56Z

@armiol @alexander-yevsyukov PTAL

armiol

@yevhenii-nadtochii very dense PR, good job! However, in some places I think you went too far. Please see my comments.

armiol · 2025-01-16T17:02:59Z

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

-// All the validation rules of a single message type.
-message MessageValidation {
+// A message compiled by the validation library with its constraints, if any.
+message CompiledMessage {


As long as in Spine there is a "Model Compiler" (which in turn heavily uses the Validation codegen capabilities), we cannot use CompiledMessage, because it clashes with that domain. Until I saw the before-after in this changeset, I had no idea what a CompiledMessage was. My only guess was that it was something describing the work of the Model Compiler routines completed for some particular Message instance, more of an item in some audit log.

I agree that MessageValidation isn't the best name. But still, what this declaration represents is by no means a result of a compilation process in that sense which we assume in Model Compiler. That's why it cannot be "compiled" (a participle, meaning "it's done"), It's rather an intermediate view of a "to-do" list for the next stages.

Let's think of a better name. I will, too.

I thought about CompiledMessage as a message that is being compiled right now. It can also be CompilingMessage, CompilationMessage.

armiol · 2025-01-16T17:15:59Z

java/src/main/kotlin/io/spine/validation/java/JavaValidationRenderer.kt

-    messageTypes
-        .map { m -> m.message.javaFile(m.fileHeader) }
-        .mapNotNull { path -> find(path) }
-        .distinct()


Are you sure that the newly introduced distinct() (line 98) is semantically equal to this whole block? I suspect there might be a subtle difference.

Now, ProtoData combines calls of javaFile(header), find(path) and unchecked cast to SourceFile<Java> in a single SourceFileSet.javaFileOf() method.

This new method throws when find(path) returns null (meaning no Java file). I have added if (!sources.hasJavaRoot) { return } in the beginning of the renderer to avoid this.

armiol · 2025-01-16T17:16:39Z

java/src/main/kotlin/io/spine/validation/java/ValidationConstraintsCode.kt

         * Creates a new instance with the generated validation constraints code.
         */
-        fun generate(r: JavaValidationRenderer, v: MessageValidation): ValidationConstraintsCode {
+        fun generate(r: JavaValidationRenderer, v: CompiledMessage): ValidationConstraintsCode {


As a general comment: when renaming something, please check the corresponding variable names, too.

armiol · 2025-01-16T17:25:25Z

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


    // `true` if the field is marked with `(required) = true`, `false` otherwise.
-    bool required = 3;
+    bool enabled = 3;


The field was called required because it's written like that in the consumer code.

"Enabled" is about something else. "Whether the validation is enabled or not?"

It is a bit weird both ways.

I suggest not create RequiredField view for non-required fields at all. The same with the rest of such views. We do nothing when such a flag is false because RequiredField with required = false is NonRequiredField.

Having this bool something = false flag in SomethingField means this something field is not something.

armiol · 2025-01-16T17:27:37Z

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


 import "spine/protodata/ast.proto";

+// Uniquely identifies a message field.


I would say it uniquely identifies a declaration of a field in some Message.

It doesn't identify the field instance itself, as one might think reading this.

It doesn't identify the field instance itself, as one might think reading this.

It does because it has both the parent type name and the field name.

armiol · 2025-01-16T17:27:56Z

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

+    // Name of the containing type.
+    protodata.TypeName type = 1;
+
+    // Short name of the field.


I think it's enough that we call the field name "short" in the declaration of the FieldName itself.

armiol · 2025-01-16T17:30:32Z

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


-    // Value of `set_once` boolean option.
+    // `true` if the field is marked with `(set_once) = true`, `false` otherwise.
    bool enabled = 3;


Basically, I would have this named set_once for that same reason as above.

armiol · 2025-01-16T17:33:55Z

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


    // Validation rules.
    //
-    // All these rules must be met in order for a message to be regarded as valid.


Why remove? The meaning is somewhat changed, don't you think?

codecov · 2025-01-21T11:31:55Z

Codecov Report

Attention: Patch coverage is 31.91489% with 32 lines in your changes missing coverage. Please review.

Project coverage is 32.16%. Comparing base (2e6ec50) to head (071b004).
Report is 27 commits behind head on master.

Additional details and impacted files

@@             Coverage Diff              @@
##             master     #185      +/-   ##
============================================
+ Coverage     31.65%   32.16%   +0.50%     
  Complexity      329      329              
============================================
  Files           142      141       -1     
  Lines          2818     2786      -32     
  Branches        229      227       -2     
============================================
+ Hits            892      896       +4     
+ Misses         1860     1824      -36     
  Partials         66       66

yevhenii-nadtochii · 2025-01-21T12:54:36Z

@armiol PTAL

armiol

@yevhenii-nadtochii LGTM with the respect of the comment.

armiol · 2025-01-21T12:58:26Z

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


 import "spine/protodata/ast.proto";

+// Uniquely identifies a field declaration within the compilation messages.


I think this type addresses not only "compilation" messages. It's a part of AST, so I say we shouldn't be specific about "compilation" messages.

yevhenii-nadtochii added 10 commits January 15, 2025 13:56

Rename view property, and move ID type up

b8bbab0

Add subject to RequiredField view

b2b1f91

Adjust the view to state change

15e9ad5

Do not narrow down visibility

472573a

Fix a typo

8b932c7

Collapse unnecessary expanded methods

b01e890

Use property instead of function

d834bc2

Rename a variable

5362e30

Make file modifications on the same level

ffa6b0f

Remove no longer used Validations

5e2f354

yevhenii-nadtochii self-assigned this Jan 16, 2025

yevhenii-nadtochii added 5 commits January 16, 2025 16:09

Actualize docs

ae1ac20

Rename MessageValidation to CompiledMessage

0d4e8d5

Bump the version -> 2.0.0-SNAPSHOT.189

32d3396

Update reports

0053a63

Proofread docs

20dade9

yevhenii-nadtochii force-pushed the refactor-entry-point branch from ae6d631 to 20dade9 Compare January 16, 2025 16:17

Fix a typo

715bbee

yevhenii-nadtochii marked this pull request as ready for review January 16, 2025 16:29

yevhenii-nadtochii requested review from alexander-yevsyukov and armiol January 16, 2025 16:29

armiol requested changes Jan 16, 2025

View reviewed changes

yevhenii-nadtochii added 7 commits January 21, 2025 11:28

Adjust the variable name to its type

cc49ba4

Rollback enabled to required

adc9f7d

Rename SetOnceField.enabled to set_once

92b6dbd

Rollback the comment

0c5709f

Update build time

8e2c3cc

Take the latest ProtoData

b0c9d5e

Expose typeSystem in JavaValidationRenderer

99af1df

yevhenii-nadtochii added 2 commits January 21, 2025 13:35

Rename CompiledMessage to CompilationMessage

6f78584

Mention that FieldId is about a declaration

0af997f

yevhenii-nadtochii requested a review from armiol January 21, 2025 12:54

armiol approved these changes Jan 21, 2025

View reviewed changes

Do not mention a particular usage context of FieldId

071b004

yevhenii-nadtochii merged commit 9748a15 into master Jan 21, 2025
6 checks passed

yevhenii-nadtochii deleted the refactor-entry-point branch January 21, 2025 13:11


		import "spine/protodata/ast.proto";

		// Uniquely identifies a message field.


		import "spine/protodata/ast.proto";

		// Uniquely identifies a field declaration within the compilation messages.

Conversation

yevhenii-nadtochii commented Jan 16, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

yevhenii-nadtochii commented Jan 16, 2025

Uh oh!

armiol left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

codecov bot commented Jan 21, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

yevhenii-nadtochii commented Jan 21, 2025

Uh oh!

armiol left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

yevhenii-nadtochii commented Jan 16, 2025 •

edited

Loading

codecov bot commented Jan 21, 2025 •

edited

Loading