[NIFI-10754] Initial check in of new getUri NIFI Expression Language …

[dan-s1]

…function.

Summary

NIFI-10754

Tracking

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

Issue Tracking

• Apache NiFi Jira issue created

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 8
  • JDK 11
  • JDK 17

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

[dan-s1]

@exceptionfactory If you have time, can you please review this in order for this to get into 1.19?

[exceptionfactory]

Thanks the contribution @dan-s1, the general approach makes sense on a cursory review, but I would like to take a closer look, so I don't think this will make it in to 1.19.0.

[dan-s1]

@exceptionfactory Now that 1.19.0 has been released, when you have some time can you please review this?

[dan-s1]

@exceptionfactory Wondering if this could be revisited. It is functionality my team could use. Thanks!

[exceptionfactory]

Thanks for working on this new feature @dan-s1. Although most of the implementation makes sense, it raises some questions about treating blanks strings as nulls. Although the java.net.URI has a number of constructors, it seems like this may provide more options than necessary. There is some value in simplicity, versus exposing all the possible options, which could also avoid potential confusion related to handling blank strings as nulls.

The discussion on NIFI-10754 originated out of issues with the urlEncode function. If the implementation were to be scoped down to just something like uriEncodePath, for instance, would that provide the desired functionality?

[exceptionfactory]

The StringUtils.isBlank() check would convert both empty strings such as "" as well as strings with spaces such as " " to null. Although these seem to be unlikely arguments, it raises a larger question about passing empty arguments. The check should probably be simplified to check for an empty string:

Suggested change

	.map(string -> StringUtils.isBlank(string) ? null : string)
	.map(string -> string == null || string.isEmpty() ? null : string)

[dan-s1]

Thanks for working on this new feature @dan-s1. Although most of the implementation makes sense, it raises some questions about treating blanks strings as nulls. Although the java.net.URI has a number of constructors, it seems like this may provide more options than necessary. There is some value in simplicity, versus exposing all the possible options, which could also avoid potential confusion related to handling blank strings as nulls.

The discussion on NIFI-10754 originated out of issues with the urlEncode function. If the implementation were to be scoped down to just something like uriEncodePath, for instance, would that provide the desired functionality?

@exceptionfactory If I understand you correctly that means there would have to be potentially at least 3 method calls when constructing a URI, one to urlEncodePath to encode the path, one to urlEncode to encode the parameters and one to concatenate all the strings necessary for creating the URI . I was actually intending to simplify that process of URI construction with one call which would take care of the encoding for both the path and the parameters and building the URI.

[exceptionfactory]

Thanks for clarifying the intent @dan-s1, constructing the complete URI in a single function makes sense.

With that background, do you need all of the possible constructor options? It might be simpler to support just one or two options, as opposed to all of the convenience constructors.

To the other question, is there a possibility that an empty string should be allowed as an argument instead of treated as null? That kind of implicit conversion to null seems like it could create confusion, and might actually be a reason to provide a different set of supported arguments so that particular values can be null when passed to the URI constructors.

[dan-s1]

@exceptionfactory

Thanks for clarifying the intent @dan-s1, constructing the complete URI in a single function makes sense.

With that background, do you need all of the possible constructor options? It might be simpler to support just one or two options, as opposed to all of the convenience constructors.

That is a hard call because I am not sure what different people need.

To the other question, is there a possibility that an empty string should be allowed as an argument instead of treated as null? That kind of implicit conversion to null seems like it could create confusion, and might actually be a reason to provide a different set of supported arguments so that particular values can be null when passed to the URI constructors.

Per the URI javadocs

a component of the new URI may be left undefined by passing null for the corresponding parameter.

I did not see a way of explicitly passing null except if a blank string was passed. Please let me know otherwise if null can be passed.

[exceptionfactory]

Thanks for the reply @dan-s1.

As you observed, the Expression Language parser does not support a literal null as a function argument. It would take a bit more work, but it seems worth evaluating what it would take to support a literal null argument for getUri. It seems like it could be scoped to this function.

For getUri itself, there does not seem to be much value in the three-argument non-hierarchical option for mailto and similar links. This could be added later, but I think it makes more sense to focus on the hierarchical URI constructors unless you have a specific use case in mind for non-hierarchical URIs.

For hierarchical URIs, the four and five argument constructors appear to fall into the convenience category. For simplicity of the initial implementation, what do you think of limiting the implementation to the seven argument constructor? Although it is the most verbose, it provides the clearest implementation for documentation and usage.

[dan-s1]

@exceptionfactory I am fine with only implementing the seven argument constructor. What did you have in mind for implementing a literal null as a function argument? I only question if it is more clearer than specifying to users to use an empty string which we can convert to null on the back end.

[exceptionfactory]

What did you have in mind for implementing a literal null as a function argument? I only question if it is more clearer than specifying to users to use an empty string which we can convert to null on the back end.

I was thinking that null would be handled as a literal along the following lines:

${getUri('https', null, 'nifi.apache.org', 8443, '/docs.html', null, null)}

An empty string could be passed to any of the string arguments, but allowing a null would avoid the need to convert the arguments and clarify expected behavior.

[dan-s1]

@exceptionfactory I added the null literal as you requested and modified the documentation to reflect this. Please let me know if my changes are what you were intending. Thanks!

[exceptionfactory]

Thanks for making the adjustments @dan-s1, this looks close to completion. I noted one more substantive change, breaking out the URI constructor arguments and port parsing to make the code more readable.

[dan-s1]

@exceptionfactory I noticed on my latest changes, the build ci-workflow / Ubuntu Zulu JDK 11 HI failed but it failed on a unit test unrelated to my changes. The unit test which failed is org.apache.nifi.processors.standard.TestHashContent
Also the ci-workflow / Ubuntu Zulu JDK 17 EN timed out. Please advise.

[exceptionfactory]

The latest PR build appears to be working as expected, it sounds like the other issues were unrelated intermittent failures.

[dan-s1]

@exceptionfactory Is there anything else that needs to be done for this PR?

[exceptionfactory]

Thanks for working through the feedback @dan-s1, the latest version looks good from a code and functional perspective. I noticed a minor documentation formatting issue with the first example expression usage, but I will correct that in the merge commit. +1 merging