Improving tests and edge cases for URI and HTTP #8497

radeusgd · 2023-12-07T18:11:45Z

Pull Request Description

Closes Follow on from the Enso Cloud API PR. #8352
~~Proposed fix for case of integer matching does not work correctly for larger numbers #8493~~
- The temporary fix is deemed not viable. I will try to figure out a workaround and leave fixing case of integer matching does not work correctly for larger numbers #8493 to the engine team.

Important Notes

Checklist

Please ensure that the following checklist has been satisfied before submitting the PR:

The documentation has been updated, if necessary.
Screenshots/screencasts have been attached, if there are any visual changes. For interactive or animated visual changes, a screencast is preferred.
All code follows the
Scala,
Java,
and
Rust
style guides. In case you are using a language not listed above, follow the Rust style guide.
All code has been tested:
- Unit tests have been written where possible.
- If GUI codebase was changed, the GUI was tested when built using ./run ide build.

radeusgd · 2023-12-07T18:14:09Z

I base this PR on top of #8403, because there I have started adding the Fetch_Spec tests extending the simple-httpbin and this PR is adding some further extensions to it - so to avoid conflicts I just developed on top of it. So, this PR should ideally be merged after #8403 gets merged.

…ix for a bug converting URI_With_Query->URI

…eries instead of rolling our own logic, pt.1

This reverts commit 4a13f3d.

… other tests

…ndlers; fixed the auth tests

JaroslavTulach

I am not convinced adding 3rd party dependency to the std-base is a good idea.

JaroslavTulach · 2023-12-15T11:40:26Z

build.sbt

-      "org.graalvm.polyglot" % "polyglot"                % graalMavenPackagesVersion,
-      "org.netbeans.api"     % "org-openide-util-lookup" % netbeansApiVersion % "provided"
+      "org.graalvm.polyglot"      % "polyglot"                % graalMavenPackagesVersion,
+      "org.apache.httpcomponents" % "httpclient"              % httpComponentsVersion,


Are we adding dependency on an external library to std-base just be able to emit HTTP request? What's wrong with JDK only solution?

We are still using JDK's httpclient for the requests (see EnsoSecretHelper.java).

The functionality that was missing was the URIBuilder that allows us to build a set of query parameters and correctly encode them. The task is not trivial as it requires escaping of various control characters depending on context (sometimes = sometimes not, & etc.) as well as escaping non-standard Unicode symbols.

We had our own simple implementation but it was missing functionalities.

Instead of trying to understand the RFC 3986 and trying to carefully implement and test that we are doing everything according to the specification, I thought it will be far simpler to use an existing solution.

If that ever turns out to be problematic, we could roll our own - but it seems like an overkill if an existing solution exists.

Note that in subsequent PRs (#8544) I plan to extend using this functionality. The URIBuilder allows us to easily parse the existing query and modify it in any way we need, as well as it allows us to parse and modify the URI path and all the other components. The JDK provides only very limited funcionality for that (e.g. we don't get the parsing of the query arugments that we need to be able to manipulate them).

Btw. http://tinyurl.com/url-escaping

There is URLEncoder which has been good enough for all my purposes so far.

I continue to be convinced that

adding dependency on (another) external library to std-base

isn't good idea.

JaroslavTulach · 2023-12-15T11:43:11Z

std-bits/base/src/main/java/org/enso/base/net/URIHelpers.java

+import java.net.URI;
+import java.net.URISyntaxException;
+import java.util.List;
+import org.apache.http.client.utils.URIBuilder;


I believe there is similar builder in the JDK as well.

How is it similar?

This is a HttpClient.Builder, I cannot see any capabilities for building an URI with it.

how did I miss it?? :O

@JaroslavTulach

- After [suggestion](#8497 (comment)) from @JaroslavTulach I have tried reimplementing the URL encoding using just `URLEncode` builtin util. I will see if this does not complicate other followup improvements, but most likely all should work so we should be able to get rid of the unnecessary bloat.

radeusgd added the CI: No changelog needed Do not require a changelog entry for this PR. label Dec 7, 2023

radeusgd self-assigned this Dec 7, 2023

radeusgd requested review from jdunkerley, GregoryTravis, 4e6, JaroslavTulach, hubertp and Akirathan as code owners December 7, 2023 18:11

radeusgd linked an issue Dec 7, 2023 that may be closed by this pull request

case of integer matching does not work correctly for larger numbers #8493

Closed

radeusgd changed the base branch from develop to wip/radeusgd/8111-big-excel-fix December 7, 2023 18:12

radeusgd force-pushed the wip/radeusgd/8352-cloud-followup-1 branch from 8cd5c51 to 9a11fc7 Compare December 8, 2023 13:33

GregoryTravis approved these changes Dec 8, 2023

View reviewed changes

radeusgd removed a link to an issue Dec 8, 2023

case of integer matching does not work correctly for larger numbers #8493

Closed

radeusgd force-pushed the wip/radeusgd/8352-cloud-followup-1 branch 2 times, most recently from 20966e9 to 3d6856b Compare December 14, 2023 12:58

jdunkerley approved these changes Dec 14, 2023

View reviewed changes

Akirathan approved these changes Dec 14, 2023

View reviewed changes

4e6 approved these changes Dec 14, 2023

View reviewed changes

Base automatically changed from wip/radeusgd/8111-big-excel-fix to develop December 15, 2023 00:02

radeusgd added 10 commits December 15, 2023 11:07

WIP

4c50256

Fetch tests

a250d3c

checkpoint

381bc29

reduce duplicate code in tests

007e928

checkpoint

7c693cb

URI_With_Query tests, extending http server to report query params; f…

3b12d28

…ix for a bug converting URI_With_Query->URI

another fix, get proper roundtrip test for query argument encoding

d7492d6

checkpoint - start relying on httpclient utils for URL encoding of qu…

44c2a5c

…eries instead of rolling our own logic, pt.1

use new logic in secrets

4b8818f

clean

823c0f8

radeusgd added 18 commits December 15, 2023 11:07

clean

02e5b49

trying to test HTTP_Error but nothing works

750d3c7

Revert "trying to test HTTP_Error but nothing works"

ce26830

This reverts commit 4a13f3d.

test also to_uri query

e018dc1

WIP testing http response code errors, some auth, response headers

f8ad5fd

checkpoint: WIP on improving handling of non-200 status codes, fixing…

0432ef8

… other tests

fixing error handling tests

78f8386

impl endpoint for the header response test

bb4c176

checkpoint

4d93f9d

WIP: auth tests

7f0d05d

make simple-httpbin easier to debug by printing stack trace in all ha…

a6956a3

…ndlers; fixed the auth tests

test crash

c590749

small test fixes

e30a46f

test multi headers

e97c6a3

simplify handling of headers in the EnsoHttpResponse

ba4c2f4

add some simple tests for JS_Object.into Map

3f54d64

javafmt

addd10b

add a conversion, add one more edge case test

f951403

radeusgd force-pushed the wip/radeusgd/8352-cloud-followup-1 branch from 3d6856b to f951403 Compare December 15, 2023 10:07

JaroslavTulach reviewed Dec 15, 2023

View reviewed changes

radeusgd added 2 commits December 15, 2023 14:12

Merge branch 'develop' into wip/radeusgd/8352-cloud-followup-1

9cd19a1

Adding licenses to the std-base Apache httpclient dependency

2230345

radeusgd added the CI: Ready to merge This PR is eligible for automatic merge label Dec 15, 2023

radeusgd added 3 commits December 15, 2023 17:06

Merge branch 'develop' into wip/radeusgd/8352-cloud-followup-1

13b4815

fix a test

bcc9572

how did I miss it?? :O

javafmt

321db41

mergify bot merged commit 940b8f7 into develop Dec 15, 2023
32 checks passed

mergify bot deleted the wip/radeusgd/8352-cloud-followup-1 branch December 15, 2023 17:58

radeusgd mentioned this pull request Dec 18, 2023

Remove the Apache dependency from std-base #8571

Merged

5 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improving tests and edge cases for URI and HTTP #8497

Improving tests and edge cases for URI and HTTP #8497

radeusgd commented Dec 7, 2023 •

edited

Loading

radeusgd commented Dec 7, 2023

JaroslavTulach left a comment

JaroslavTulach Dec 15, 2023

radeusgd Dec 15, 2023 •

edited

Loading

radeusgd Dec 15, 2023

JaroslavTulach Dec 18, 2023 •

edited

Loading

JaroslavTulach Dec 15, 2023

radeusgd Dec 15, 2023

Improving tests and edge cases for URI and HTTP #8497

Improving tests and edge cases for URI and HTTP #8497

Conversation

radeusgd commented Dec 7, 2023 • edited Loading

Pull Request Description

Important Notes

Checklist

radeusgd commented Dec 7, 2023

JaroslavTulach left a comment

Choose a reason for hiding this comment

JaroslavTulach Dec 15, 2023

Choose a reason for hiding this comment

radeusgd Dec 15, 2023 • edited Loading

Choose a reason for hiding this comment

radeusgd Dec 15, 2023

Choose a reason for hiding this comment

JaroslavTulach Dec 18, 2023 • edited Loading

Choose a reason for hiding this comment

JaroslavTulach Dec 15, 2023

Choose a reason for hiding this comment

radeusgd Dec 15, 2023

Choose a reason for hiding this comment

radeusgd commented Dec 7, 2023 •

edited

Loading

radeusgd Dec 15, 2023 •

edited

Loading

JaroslavTulach Dec 18, 2023 •

edited

Loading