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

Add ValueExtractor for ArgumentValue #737

Closed
wants to merge 3 commits into from

Conversation

bduisenov
Copy link
Contributor

@bduisenov bduisenov commented Jun 21, 2023

Validation annotations on ArgumentValue are not working as there's no ValueExtractor provided.
For example, this code ArgumentValue<@NotBlank @Size(min = 2, max = 500) String> name will not work as expected.

Adding ValueExtractor will solve the issue

@UnwrapByDefault
public class ArgumentValueExtractor implements ValueExtractor<ArgumentValue<@ExtractedValue ?>> {

    @Override
    public void extractValues(ArgumentValue<?> originalValue, ValueReceiver receiver) {
        if (originalValue.isPresent()) {
            receiver.value(null, originalValue.value());
        }
    }
}

#736

@pivotal-cla
Copy link

@bduisenov Please sign the Contributor License Agreement!

Click here to manually synchronize the status of this Pull Request.

See the FAQ for frequently asked questions.

@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged label Jun 21, 2023
@pivotal-cla
Copy link

@bduisenov Thank you for signing the Contributor License Agreement!

Copy link
Contributor

@rstoyanchev rstoyanchev left a comment

Choose a reason for hiding this comment

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

Thanks for the PR.

We probably don't want to put a helper class of this kind at the top of the data package. It could go further below under ~.data.method.annotation.support where we already have other validation support. A test to prove this works would also be nice to have, probably in ValidationHelperTests.

Let me know if you would you like to shape the changes further based on this, or if not we can take it from here.

@rstoyanchev rstoyanchev self-assigned this Jun 28, 2023
@rstoyanchev rstoyanchev added type: enhancement A general enhancement and removed status: waiting-for-triage An issue we've not yet triaged labels Jun 28, 2023
@rstoyanchev rstoyanchev added this to the 1.2.2 milestone Jun 28, 2023
@bduisenov
Copy link
Contributor Author

hey @rstoyanchev
Thanks for your feedback. I'm happy to adjust the code.

@bduisenov
Copy link
Contributor Author

hey @rstoyanchev, please take another look.

@rstoyanchev
Copy link
Contributor

Thanks for the changes.

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

Successfully merging this pull request may close these issues.

4 participants