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

NIFI-12573 Improve support for Enum values in PropertyDescriptor.Builder #8211

Closed
wants to merge 1 commit into from

Conversation

EndzeitBegins
Copy link
Contributor

@EndzeitBegins EndzeitBegins commented Jan 7, 2024

Summary

NIFI-12573
Lowers type restrictions on three methods in PropertyDescriptor.Builder to allow to work with DescribedValue instead of child class AllowableValue and with both Enum classes that do and do not implement DescribedValue.

Also addresses NIFI-12574 by adding a new method clearDefaultValue to PropertyDescriptor.Builder.

Adds several test cases for the affected code.

Tracking

Please complete the following tracking steps prior to pull request creation.

Issue Tracking

Pull Request Tracking

  • Pull Request title starts with Apache NiFi Jira issue number, such as NIFI-00000
  • Pull Request commit message starts with Apache NiFi Jira issue number, as such NIFI-00000

Pull Request Formatting

  • Pull Request based on current revision of the main branch
  • Pull Request refers to a feature branch with one commit containing changes

Verification

Please indicate the verification steps performed prior to pull request creation.

Build

  • Build completed using mvn clean install -P contrib-check
    • JDK 21

Licensing

  • New dependencies are compatible with the Apache License 2.0 according to the License Policy
  • New dependencies are documented in applicable LICENSE and NOTICE files

Documentation

  • Documentation formatting appears as expected in rendered files

@EndzeitBegins EndzeitBegins force-pushed the NIFI-12573 branch 2 times, most recently from 520a6a3 to 19bc4ac Compare January 7, 2024 15:31
@EndzeitBegins EndzeitBegins marked this pull request as ready for review January 7, 2024 16:16
NIFI-12574 Add clearDefaultValue to PropertyDescriptor.Builder
Copy link
Contributor

@exceptionfactory exceptionfactory 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 contribution @EndzeitBegins. Supporting any enum would follow a similar pattern to the existing methods that take a set of strings. Is there any other particular reason prompting these changes? In the course of new implementations, providing an enum that implements DescribedValue is always preferred because it requires methods for display name and description.

@EndzeitBegins
Copy link
Contributor Author

EndzeitBegins commented Jan 15, 2024

Thanks for the questions @exceptionfactory.
First of all I agree that it should be preferred to let the enums implement DescribedValue whenever feasible.
Maybe that's something worth to better highlight in the code documentation.

In particular I thought about two situations, where the more adaptive approach introduced in this PR would be beneficial.

  1. There might be cases we're an existing enum (e.g. from a library) is used that cannot be altered. Even though I'd personally prefer creating one's own enum (implementing DescribedValue) that maps to the library one (similar to the azure bundle) I can see how someone might not want to go "the extra mile". A user can use that Enum as allowableValues more easily now.
  2. And by far more important to me, I noticed that when using the existing public <E extends Enum<E>> Builder allowableValues(final E[] values) with an enum that actually implements DescribedValue, those values will be ignored, which might come as a surprise. I wouldn't want to restrict the Enums to implement DescribedValue here, as that would be quite a large breaking change. That's why I thought aligning the behaviour of all Enum based methods would be beneficial for ease of understanding.

@exceptionfactory
Copy link
Contributor

Thanks for the quick reply @EndzeitBegins. That's a very good point about the second scenario, and the first one is also reasonable. With that background, I agree with the changes and will proceed with merging. +1

@EndzeitBegins
Copy link
Contributor Author

As always, thank you for input and taking on the merge process @exceptionfactory. 🙂👍🏻

@EndzeitBegins EndzeitBegins deleted the NIFI-12573 branch January 15, 2024 20:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants