Skip to content

NIFI-14363 - Add a replaceByPattern expression language function#9799

Merged
exceptionfactory merged 2 commits intoapache:mainfrom
pvillard31:NIFI-14363
Mar 13, 2025
Merged

NIFI-14363 - Add a replaceByPattern expression language function#9799
exceptionfactory merged 2 commits intoapache:mainfrom
pvillard31:NIFI-14363

Conversation

@pvillard31
Copy link
Copy Markdown
Contributor

@pvillard31 pvillard31 commented Mar 12, 2025

Summary

NIFI-14363 - Add a replaceByPattern expression language function

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

Signed-off-by: Pierre Villard <pierre.villard.fr@gmail.com>
Copy link
Copy Markdown
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 proposing this new function @pvillard31. The basic concept makes sense, but I think a different name would be more intuitive.

The mapTo name seems to imply the argument contains the destination, reminding me of the Kotlin mapTo functions. Just using map seems too generic, but something that incorporates that word is a good starting point.

Options such as mapKeyValue or mapKeyPatternValue come to mind, although the latter is more verbose. Even mapKeyValue isn't the most clear.

One other concern is the , separator, which could be problematic for matching , characters in the expression itself.

One that comes to mind is altering the approach to take only one pattern and one value at a time. This would require multiple function calls, but could be cleaner. Could the desired behavior accomplished using multiple replace or replaceFirst calls? Perhaps building on the replace naming would be a better way to go.

Those are some initial thoughts, glad to discuss this further to consider the best solution.

@pvillard31 pvillard31 changed the title NIFI-14363 - Add a mapTo expression language function NIFI-14363 - Add a replaceByPattern expression language function Mar 13, 2025
@pvillard31
Copy link
Copy Markdown
Contributor Author

Thanks for the review @exceptionfactory - I pushed a commit to rename the function into replaceByPattern which seems to be a better fit.

Regarding the , separator concern, I think this is a very rare edge case where the user could use a more advanced combination of EL functions to get the desired result. An option that comes to mind is to accept an optional second argument to let the user specify the delimiter. This could be a follow up improvement if needed.

Copy link
Copy Markdown
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 making the adjustments @pvillard31. I think replaceByPattern is a good fit for the functionality, and I agree the , separator is somewhat of an edge case that could be addressed in the future with an additional optional argument providing an alternative delimiter if needed.

I plan to merge pending successful builds.

@exceptionfactory exceptionfactory merged commit d46ee93 into apache:main Mar 13, 2025
6 checks passed
// if the search string is a literal, we don't need to evaluate it each time; we can just
// pre-compile it. Otherwise, it must be compiled every time.
if (search instanceof StringLiteralEvaluator) {
this.compiledPatterns = compilePatterns(search.evaluate(new StandardEvaluationContext(Collections.emptyMap())).getValue());
Copy link
Copy Markdown
Contributor

@dan-s1 dan-s1 Mar 13, 2025

Choose a reason for hiding this comment

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

This maybe a little late but I do not think you need Collections.emptyMap() for this, couldn't you just use Map.of() ?

Suggested change
this.compiledPatterns = compilePatterns(search.evaluate(new StandardEvaluationContext(Collections.emptyMap())).getValue());
this.compiledPatterns = compilePatterns(search.evaluate(new StandardEvaluationContext(Map.of())).getValue());

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thanks @dan-s1, they are functionally equivalent, but I agree Map.of() is preferred. I just merged the PR, but good to keep in mind for the future.

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.

3 participants