Skip to content
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

Generate Java validation code at compile time #497

Merged
merged 230 commits into from
Dec 30, 2019

Conversation

dmdashenkov
Copy link
Contributor

@dmdashenkov dmdashenkov commented Nov 12, 2019

This PR starts the process of moving Java Protobuf validation code to build stage.

Before this PR, all the validation work was done at runtime. This included assembling metadata, scanning classpath for custom validation strategies, and, last but least, destructing messages into fields via costly reflective calls.

Starting from now, we perform most of the validation work at build time. The runtime only invokes the functions for validating a specific message type.

Performance

As measured on load tests in core-java, the performance impact of validation has been cut significantly.
Here are the measurements for a test which dispatches thousands of messages to several entities:

Running mode CPU time, ms GC calls per test Max RAM usage, MB
Old runtime 2000-3000 n/a n/a
New runtime 1700-2500 ~2 ~1800
Generated 1200-2000 ~1 ~1600

API status

Currently, this feature is in beta. The old runtime-only validation is still functional. To switch to the new implementation, configure the code generation in Gradle:

modelCompiler {
    generateValidation = true
}

The base types are now supplied with the generated validation, as will be time and core types after those modules are migrated to the new version of base.

Interop

When invoking generated validation, the runtime validation may still be used for types for which the validation code was not generated. Note that the vice-versa interop does not work, i.e. runtime validation never invokes generated validation code.

Limitations

It is planned that the generated validation will cover all the runtime validation features and effectively replace it in future releases. For now, external validation rules (i.e. (validation_for)) are not supported.

Behaviour changes

Previously, when validating a string field, the value was checked upon the pattern constraint even if the string was empty. This was inconsistent with all the other validation. Now, the string value is only checked if it's not empty.

@codecov
Copy link

codecov bot commented Nov 12, 2019

Codecov Report

Merging #497 into master will decrease coverage by 0.58%.
The diff coverage is 82.16%.

@@             Coverage Diff              @@
##             master     #497      +/-   ##
============================================
- Coverage     72.83%   72.24%   -0.59%     
- Complexity     2614     2681      +67     
============================================
  Files           454      467      +13     
  Lines         10589    11036     +447     
  Branches        616      626      +10     
============================================
+ Hits           7712     7973     +261     
- Misses         2700     2845     +145     
- Partials        177      218      +41

@dmdashenkov dmdashenkov changed the title Generate Kotlin validation code at compile time Generate Java validation code at compile time Nov 20, 2019
@dmdashenkov
Copy link
Contributor Author

@armiol, @alexander-yevsyukov, PTAL.

Copy link
Contributor

@alexander-yevsyukov alexander-yevsyukov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's one question regarding value out of range. If you can address that, please do.
Other than that LGTM.

return value.substring(1, value.length() - 1);
@Override
protected String compileErrorMessage(Range<ComparableNumber> range) {
return format("The value of the field `%s` is out of range. Must be %s%s and %s%s.",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've updated the language here myself, removing the word “between” which was redundant, assuming that following text says “less than”. But there's one more thing about this error message. What is the value, which is out of range?

I think, we need to tell it to make finding the problem easier. Can we do that?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is possible to add the field value to the message. (min) and (max) also don't provide the value in the message. However, in both cases (of (range) and (min)/(max)) provide the field value in the violation body.
I'd rather leave this behaviour as is. This gives the users an additional perk by making the error message non-sensitive, i.e. such that does not contain potentially sensitive data.


/**
* Obtains the expression which calls {@code Messages.isDefault())} method on the given
* {@code value}.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A common issue with Javadocs we have is widow words at the end of some paragraphs.

I'm not suggesting to review and fix it throughout the Javadocs now (especially assuming this big and important PR). I just want to tell you about such a thing so that we can start improving this aspect of the documentation in future works.

…e/base into generate-java-validation

� Conflicts:
�	base/src/main/java/io/spine/validate/option/RangeConstraint.java
@dmdashenkov
Copy link
Contributor Author

dmdashenkov commented Dec 27, 2019

@armiol, PTAL.

@dmdashenkov dmdashenkov merged commit 37d9114 into master Dec 30, 2019
@dmdashenkov dmdashenkov deleted the generate-java-validation branch December 30, 2019 11:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants