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

Disappearing (validate) constraint #1564

Draft
wants to merge 1 commit into
base: class-based-routing
Choose a base branch
from

Conversation

yevhenii-nadtochii
Copy link
Contributor

@yevhenii-nadtochii yevhenii-nadtochii commented Jan 9, 2025

This PR showcases unexpected behavior of codegen validation in core-java. Please note, it is based on class-based-routing branch.

For some reason, when (if_invalid) is applied for Subscription.topic field, its generated code doesn't contain the (validate) check in validate() method.

In StandTest, the modified line should have contained buildPartial() from the very beginning.

Take a look at comparison of the generated validate() method with and without (if_invalid) option:

image

Discovered in no longer needed #1563.

@yevhenii-nadtochii
Copy link
Contributor Author

@alexander-yevsyukov PTAL

I mentioned this question in the morning.

@yevhenii-nadtochii
Copy link
Contributor Author

yevhenii-nadtochii commented Jan 10, 2025

@alexander-yevsyukov I've reproduced it in validation.

Caused by the use of the deprecated (if_invalid).msg_format instead of (if_invalid).error_msg. Hard to notice. As a result, the error message remained empty, whereas (if_invalid) was discovered by the corresponding projection and expected a non-empty message in error_msg. The validation library throws an exception in this case.

There are the following problems:

  1. It looks like we are going to keep msg_format for first releases of V2, so it either should be a somehow workable option with a warning, or an explicit compilation error.
  2. Runtime exceptions from validation are hidden, but they are actually compilation errors.
  3. Deprecation warnings from protoc are hidden, if they exist at all.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant