HTTP-78 Support for optional query parameters for body-based requests #79

OlivierZembri · 2024-03-15T14:58:13Z

Support for optional query parameters for body-based requests

Description

As documented, the connector currently create a lookup query from the HTTP TableLookup Source to build either:

a query parameter list in case of a HTTP GET request
a body payload in case of a HTTP body-based request

There is no way to change this behaviour with custom factories.

Modification:

Change the signature of createLookupQuery(RowData lookupDataRow) in interface LookupQueryCreator so that it returns a class LookupQueryInfo holding:
- the lookup query
- the optional body-based query parameters
Adapt the creation of the URIs in the constructions of HTTP requests to retrieve information from the class LookupQueryInfo.

With these changes, any custom implementation of this interface that aims to provide body-based request is able to provide the lookup query as the payload and an optional formatted string representing the query parameters.

Resolves #78

PR Checklist

Tests added
Changelog updated

kristoffSC

Looking good but I have few nits and questions.

kristoffSC · 2024-03-21T08:47:45Z

src/main/java/com/getindata/connectors/http/internal/table/lookup/LookupQueryInfo.java

+    public LookupQueryInfo(String lookupQuery, Map<String, String> bodyBasedUrlQueryParams) {
+        this.lookupQuery =
+                Optional.ofNullable(lookupQuery)
+                        .orElseGet(() -> "");


you can use orElse("") instead orElseGet

although your proposal is equivalent to what I wrote, there would be a performance penalty to use orElse("") instead orElseGet as explained here: https://www.baeldung.com/java-optional-or-else-vs-or-else-get#orElseGet

The link you've provided explains when orElse would introduce a performance penalty - that is, when you're calling a method inside the orElse() method, especially not straight forward one like random.
This performance hit comes because in case of orElse we're always calling that method, no matter if we need it or not. In case of orElseGet, we just call it when we actually need it. This is the only difference. Please refer to the implementation of both methods to confirm the above.
And so here, where a constant is used (see Java String Pool), there should be no performance difference whatsoever.

Fixed in eba1be7#diff-5d0d1d1012fdd57bd2aa4b4fe50dca8d1ce442236c2eea7283222092d917b4a4R35-R40

kristoffSC · 2024-03-21T08:48:24Z

src/main/java/com/getindata/connectors/http/internal/table/lookup/LookupQueryInfo.java

+                        .orElseGet(() -> "");
+        this.bodyBasedUrlQueryParams =
+                Optional.ofNullable(bodyBasedUrlQueryParams)
+                        .orElseGet(() -> new LinkedHashMap<>());


please consider using .orElse(Collections.emptyMap())

the same performance penalty mentioned in comment #79 (comment) would come into play

but I understand your point to use a read-only collection with Collections.emptyMap()

Same as above - we're passing a constant here, there should be no difference in performance.

Indeed, the Collections.emptyMap() uses a static member compared to new LinkedHashMap<>()

Fixed in eba1be7#diff-5d0d1d1012fdd57bd2aa4b4fe50dca8d1ce442236c2eea7283222092d917b4a4R35-R40

kristoffSC · 2024-03-21T08:51:41Z

src/main/java/com/getindata/connectors/http/internal/table/lookup/LookupQueryInfo.java

+    }
+
+    public String getBodyBasedUrlQueryParameters() {
+        return URLEncodedUtils.format(


what would be returned if bodyBasedUrlQueryParams would be empty?
A null or empty string?

An empty string is returned

kristoffSC · 2024-03-21T09:04:24Z

src/main/java/com/getindata/connectors/http/internal/table/lookup/BodyBasedRequestFactory.java

+        if (lookupQueryInfo.hasBodyBasedUrlQueryParameters()) {
+            resolvedUrl.append(baseUrl.contains("?") ? "&" : "?")
+                       .append(lookupQueryInfo.getBodyBasedUrlQueryParameters());
+        }


I'm guessing you are planing to have your own LookupQueryCreator custom implementation that will return a LookupQueryInfo that will have both, query and body params right?

And you plan to use it with one of the HttpRequestFactory implementation right?

I wonder about contract between LookupQueryInfo and HttpRequestFactory.
In other words I wonder if the logic above is an implementation detail of LookupQueryInfo or public contract that LookupQueryInfo creator should know.

I'm guessing you are planing to have your own LookupQueryCreator custom implementation that will return a LookupQueryInfo that will have both, query and body params right?

Yes, that's the final purpose:

having POST requests with body parameters only

having POST requests with body parameters and query parameters

In other words I wonder if the logic above is an implementation detail of LookupQueryInfo

In my view, this is rather an implementation detail and the HttpRequestFactory can be left untouched thereby remaining the "happy path".

kristoffSC · 2024-03-21T20:39:42Z

src/main/java/com/getindata/connectors/http/internal/table/lookup/LookupQueryInfo.java

+        this.bodyBasedUrlQueryParams =
+                Optional.ofNullable(bodyBasedUrlQueryParams)
+                        .orElse(Collections.emptyMap());
+    }


Actually I don't think optional are the best thing to use here.
We are not gaining much from them and I would argue they are used not as Optionala were intend to be used.

I see that you are using them to have "nicely looking" null checks, but I think below code would do the same thing and will not be much harder do read:

this.lookupQuery = lookupQuery == null ? "" : lookupQuery; this.bodyBasedUrlQueryParams = bodyBasedUrlQueryParams == null ? Collections.emptyMap() : bodyBasedUrlQueryParams;

Please substitute Optionals with the above code.

No problem, change done in d0e1822#diff-5d0d1d1012fdd57bd2aa4b4fe50dca8d1ce442236c2eea7283222092d917b4a4R34-R38

Support for optional query parameters for body-based requests Signed-off-by: Olivier Zembri <<ozembri@fr.ibm.com>>

davidradl · 2024-03-22T12:57:16Z

src/main/java/com/getindata/connectors/http/internal/table/lookup/BodyBasedRequestFactory.java

@@ -53,9 +53,15 @@ protected Logger getLogger() {
        return log;
    }

-    private URI constructGetUri() {
+    URI constructBodyBasedUri(LookupQueryInfo lookupQueryInfo) {


Small comment, I was confused by the name of this method. I wonder if constructUri or addQueryInfoToURI would be more appropriate.
When we add path parameters I assume there would be another map in LookupQueryInfo that we would use to replace content in the parameterised baseUrl.

When we add path parameters

This is not the scope of the current PR. It might be revisited with a PR dedicated to path parameters.

When we add path parameters

This is not the scope of the current PR. It might be revisited with a PR dedicated to path parameters.

OK

…getindata#79) Support for optional query parameters for body-based requests Signed-off-by: Olivier Zembri <<ozembri@fr.ibm.com>>

OlivierZembri force-pushed the issue-78 branch from ae11a89 to abd7679 Compare March 15, 2024 15:06

kristoffSC self-requested a review March 17, 2024 21:46

kristoffSC self-assigned this Mar 17, 2024

kristoffSC added the enhancement New feature or request label Mar 17, 2024

OlivierZembri force-pushed the issue-78 branch from abd7679 to 9cd40ba Compare March 18, 2024 08:21

kristoffSC reviewed Mar 21, 2024

View reviewed changes

OlivierZembri force-pushed the issue-78 branch from 9cd40ba to eba1be7 Compare March 21, 2024 12:27

kristoffSC reviewed Mar 21, 2024

View reviewed changes

HTTP-78 Support for optional query parameters for body-based requests

d0e1822

Support for optional query parameters for body-based requests Signed-off-by: Olivier Zembri <<ozembri@fr.ibm.com>>

OlivierZembri force-pushed the issue-78 branch from eba1be7 to d0e1822 Compare March 22, 2024 06:56

davidradl reviewed Mar 22, 2024

View reviewed changes

kristoffSC approved these changes Mar 22, 2024

View reviewed changes

kristoffSC merged commit b17ede3 into getindata:main Mar 22, 2024
2 of 3 checks passed

davidradl mentioned this pull request May 20, 2024

Allow GET POST PUT parameter resolution through configuration #99

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

HTTP-78 Support for optional query parameters for body-based requests #79

HTTP-78 Support for optional query parameters for body-based requests #79

OlivierZembri commented Mar 15, 2024

kristoffSC left a comment

kristoffSC Mar 21, 2024

OlivierZembri Mar 21, 2024

mschielmann Mar 21, 2024

OlivierZembri Mar 21, 2024

kristoffSC Mar 21, 2024

OlivierZembri Mar 21, 2024

OlivierZembri Mar 21, 2024

mschielmann Mar 21, 2024

OlivierZembri Mar 21, 2024

OlivierZembri Mar 21, 2024

kristoffSC Mar 21, 2024

OlivierZembri Mar 21, 2024

kristoffSC Mar 21, 2024

OlivierZembri Mar 21, 2024

kristoffSC Mar 21, 2024 •

edited

Loading

OlivierZembri Mar 22, 2024

davidradl Mar 22, 2024

OlivierZembri Mar 22, 2024

davidradl Mar 22, 2024

HTTP-78 Support for optional query parameters for body-based requests #79

HTTP-78 Support for optional query parameters for body-based requests #79

Conversation

OlivierZembri commented Mar 15, 2024

Description

PR Checklist

kristoffSC left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kristoffSC Mar 21, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kristoffSC Mar 21, 2024 •

edited

Loading