-
Notifications
You must be signed in to change notification settings - Fork 0
Support custom message validators #224
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
# Conflicts: # dependencies.md # pom.xml # version.gradle.kts
# Conflicts: # dependencies.md # pom.xml # version.gradle.kts
armiol
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@yevhenii-nadtochii please see my comments so far.
| /** | ||
| * A custom validator for Protobuf messages of type [M] that are defined externally. | ||
| * | ||
| * External messages are those that come from dependencies. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We consider "external" those Message definitions for which our end-users do not generate the Java/Kotlin code.
The definition you give is just misleading because end-users can generate code for any Proto type, including those coming from any third-party dependency. Many libraries come with their own generated and compiled Java code for their Proto definitions (such as Google Cloud libraries). And it is pretty difficult to generate another version of Java code and still make the libraries work properly.
I think the beginning (maybe even including the first paragraph) should be updated accordingly. Let's consider the readers much less prepared.
| * | ||
| * ## Applicability | ||
| * | ||
| * A validator is applied only to the local messages of the module it is declared in. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't understand what you are trying to say here.
|
@armiol PTAL |
armiol
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@yevhenii-nadtochii please see more comments on the same matter.
| val result = compilation.compileSilently() | ||
| result.exitCode shouldBe OK | ||
|
|
||
| val output = compilation.kspSourcesDir |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What if the DiscoveredValidators have the API on performing these resolve ops, instead of being just a holder for some constant?
| * | ||
| * **Local messages** are message types for which end-users generate and control | ||
| * the Java/Kotlin classes by compiling the corresponding `.proto` definitions | ||
| * within their own codebase. For such messages, the validation library allows |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"Validation". It's a name.
| /** | ||
| * A custom validator for an external Protobuf message of type [M]. | ||
| * | ||
| * ## Notation of external and local messages |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"## "External" and "local" `Message`s types
| import io.spine.annotation.SPI | ||
|
|
||
| /** | ||
| * A custom validator for an external Protobuf message of type [M]. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A custom validator ...
Do we have our own implementations of this MessageValidator? I mean, is it expected that all implementations are for the custom "external" Message types only?
alexander-yevsyukov
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please see my comments.
| * ## Validation of external messages | ||
| * | ||
| * The validation library provides a customization mechanism that allows validating | ||
| * of the external messages, **which are embedded within local messages**. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The "embedded" sounds a bit confusing in this context. Why don't we tell that these message types are used for fields in the local messages?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I mean "embedded" to me means that a message type is nested under another message type. This is not possible in the case we describe here. So, the reader has to conclude it to himself and then "decipher" it to the usage scenario I described above.
| * of [DetectedViolation]. Before reporting to the user, the validation library | ||
| * converts [DetectedViolation] to a [ConstraintViolation][io.spine.validate.ConstraintViolation]. | ||
| * Returning of an empty list of violations means that the message is valid. | ||
| * Returning of an empty list of violations means that the passed message is valid. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's replace "passed" with "given".
armiol
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@yevhenii-nadtochii getting closer. Please see my comments.
| /** | ||
| * A validator for an external Protobuf message of type [M]. | ||
| * | ||
| * This interface allows adding validation logic to messages, for which there is |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, we don't add any logic to messages which we don't control.
We enforce the validation rules for Messages that have Java/Kotlin code already generated by a third party, in case they are used in the Proto codebase to which the Validation library is applied.
| * ## Problem | ||
| * | ||
| * Java/Kotlin libraries that use Protobuf messages often distribute both the `.proto` | ||
| * definitions and the compiled class files (.class) for these messages. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
`.class`
| * | ||
| * 1) It is used to validate a local message. Only external messages are allowed | ||
| * to have a validator. | ||
| * 2) There already exists a validator for the specified message type. Having several |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's also describe the scope in which these rules apply. E.g. one may think it's scoped by a Gradle module, while it is certainly not.
Please have it somewhere above, as it is an important point.
| * | ||
| * ## Implementation | ||
| * | ||
| * The message validator does not have restrictions upon how exactly the message |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's start with a positive sentence, not with the one built around the negation.
alexander-yevsyukov
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
|
@armiol PTAL |
This PR brings support for the custom
MessageValidators.The feature specification resides in the docs to the
MessageValidatorinterface.This feature is based around two main entities residing in the
:java-apimodule:MessageValidator<T : Message>– an interface that defines behavior of the validators.@Validator(T::class)– an annotation used for the discovering of the validators by the library.The following modules were added:
:java-ksp– responsible for discovering of the annotated validators. The module outputs the discovered validators to a plain text file within the task output directory of KSP.:java-tests:validator-dependency– declares an external message for tests.:java-tests:validator– tests validators with external and well-known Protobuf messages.JavaValidationPluginfetches the file left by the KSP task, and passes the class names of the validators and their target messages to the renderer. The renderer bypasses them toValidatorGenerator, which generates the actual invocations.Note that
ValidatorGeneratordoes not belong to the hierarchy of the option generators. It stands on its own for the following reasons:MessageTypeinstead ofTypeNamebecause it needs to go through the message fields.Additional changes
Added an extension
fun <T : Dependency> T.artifact(module: T.() -> String): String, which allows using the properties of the targetDepenencyobject without referencing the object itself.