-
Notifications
You must be signed in to change notification settings - Fork 0
Make the Java validation extendable #215
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
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #215 +/- ##
============================================
- Coverage 32.65% 32.04% -0.62%
- Complexity 236 238 +2
============================================
Files 119 128 +9
Lines 2811 2896 +85
Branches 205 208 +3
============================================
+ Hits 918 928 +10
- Misses 1841 1916 +75
Partials 52 52 🚀 New features to boost your workflow:
|
@armiol @alexander-yevsyukov PTAL As for now, I've made Regarding extension points. We have one for both language-agnostic model classes (policies, views) AND Java-specific option generators - This way, there are three steps:
|
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 the first-round comments.
I also think we need a separate module (and artifact) which clearly declares the public API for SPI users. Once this is done, we will need to update the documentation with this regard, and mark some types as @SPI
.
As discussed, we will meet today on the call, and talk over the remaining points.
@@ -0,0 +1,59 @@ | |||
/* | |||
* Copyright 2024, TeamDev. All rights reserved. |
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 Java class of the option [view][View]. | ||
*/ | ||
public val view: Class<out View<*, *, *>> |
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 allow having multiple view
and policy
.
@@ -90,5 +96,5 @@ public abstract class ValidationPlugin(renderers: List<Renderer<*>> = emptyList( | |||
SetOncePolicy(), | |||
IfSetAgainPolicy(), | |||
RequirePolicy() | |||
) | |||
) + policies |
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 review this file for code formatting.
@@ -51,7 +52,12 @@ import io.spine.validation.required.RequiredPolicy | |||
* The concrete implementations should provide [renderers], which implement | |||
* these constraints for a specific programming language. | |||
*/ | |||
public abstract class ValidationPlugin(renderers: List<Renderer<*>> = emptyList()) : Plugin( | |||
public abstract class ValidationPlugin( |
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.
Having this type as public
is now confusing. Given we don't suggest that end-users create their own plugins, but rather extend via CustomOption
, this type should be @Internal
at the very least.
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.
It was public
before, I haven't changed its visibility.
It is public
for consumers of the :model
module, which are language-specific plugins.
/** | ||
* Injects [Querying] into this instance of [OptionGenerator]. | ||
*/ | ||
internal fun inject(querying: Querying) { |
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 mark this @Internal
.
# Conflicts: # dependencies.md
@yevhenii-nadtochii is this a draft or something ready for reviewing? |
This is a draft. |
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 LGTM.
In the following rounds we'll need to review the newly created module for clarity. However, it is easier to do in the IDE rather than via the GitHub PR review.
In the meantime, could you please append the repo-level README.md
with some notes on how to provide own custom options.
This PR makes the validation library extendable for Java users.
Now, users can create policies, views and Java generators for their custom Protobuf options.
ValidationOption
interfaceUsers can create their own providers of policies, views and Java generators by extending from the
ValidationOption
interface, and marking their implementation with@AutoService(ValidationOption::class)
.At the creation of
JavaValidationPlugin
, all implementations ofValidationOption
are loaded and their views, policies and generators are being added to the plugin.Generator dependencies
Previously, all generators were created right within a member of a bounded context.
JavaValidationRenderer
created them passing all dependencies they needed, including those provided by the context.Now, instances of generators must be created before they get into the context. They are firstly passed to
ValidationOption
, and later bypassed to the context member.As for now, there are two dependencies that can be provided only by a member of a bounded context:
Querying
– from our experience, every code generator needs this class to be able to fetch views, upon which the generator operators. Again, custom generators are created outside the context member. Thus,Querying
must be injected after the generator gets into the context. For this,OptionGenerator
is now an abstract class that haslateinit var querying: Querying
populated later by the renderer.EmptyFieldCheck
→JavaValueConverter
→TypeSystem
. The generators useEmptyFieldCheck
trait to check arbitraryField
s for emptiness, which introduces a transitive dependency onTypeSystem
, which is a member property. We can handleTypeSystem
just asQuerying
and allow custom options to utilizeEmptyFieldCheck
as is OR address UpdateEmptyFieldCheck
to avoid usage ofJavaValueConverter
#199 because this class can be re-implemented withoutTypeSystem
dependency. ProtoData events now have everything needed for codegen, the lacking information is fetched by policies. At the stage of codegen, the type system must not be used conceptually. So, I suggest addressing the mentioned issue instead of bypassingTypeSystem
to the generators.Represent current options as a set of
ValidationOption
implementationsIn general, it looks possible. Each option now is a combination of a generator + view + policy.
But having them as a set of
ValidationOption
s breaks our separation of language-agnostic model and language-specific generators. This separation exists on the level of the validation library, but it is not reflected in theValidationOption
interface, which is Java-centric.:java-api
moduleAll re-usable members of the
:java
and:model
modules now reside in:java-api
module. TheValidationOption
interface is also part of the API module. To implement a custom option, only the API module is needed.MoneyValidationPlugin
MoneyValidationPlugin
that handled a custom(currency)
option withRule
s becameCurrencyOption
.Now, it is represented with a generator, a policy and a view.