Merge `URI_With_Query` into `URI`, extend API of `URI` #8591

radeusgd · 2023-12-19T17:39:24Z

Pull Request Description

Closes Merge URI with URI_With_Query #8544
Adds reset_query_arguments and / operators allowing to transform a URI.
Adding tests for handling of various edge cases.

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.

JaroslavTulach

Removing duplication is good. I'd suggest some more removal, but I don't see the whole picture.

JaroslavTulach · 2023-12-20T04:05:16Z

distribution/lib/Standard/Base/0.0.0-dev/src/Data/Enso_Cloud/Enso_Secret.enso

@@ -87,6 +89,12 @@ type Enso_Secret
    exists name:Text parent:(Enso_File | Nothing)=Nothing =
        Enso_Secret.list parent . any s-> s.name == name

+    ## PRIVATE
+    to_java_value : Text | Enso_Secret | Nothing -> HideableValue


Things related to security should really be private not just documented ## PRIVATE, in my opinion. E.g. move the method into own private module, make it an extension method.

I don't think I can do this, because Enso_Secret.to_java_value is used within Standard.Database, outside of Standard.Base.

I can move it to a separate file but I cannot make it inaccessible from other modules.

Note that this does not expose any 'sensitive' data - it just creates a Java representation of the Enso_Secret value, using its payload. It's just a transformation of existing data into an internal format.

I have moved this method to Enso_Cloud/Utils and renamed it to as_hideable_value

JaroslavTulach · 2023-12-20T04:07:32Z

distribution/lib/Standard/Base/0.0.0-dev/src/Network/Extensions.enso

@@ -99,43 +81,3 @@ URI_With_Query.fetch self (method:HTTP_Method=HTTP_Method.Get) headers=[] try_au
 URI.post : Request_Body -> HTTP_Method -> Vector (Header | Pair Text Text) -> Boolean -> Any
 URI.post self (body:Request_Body=Request_Body.Empty) (method:HTTP_Method=HTTP_Method.Post) (headers:(Vector (Header | Pair Text Text))=[]) (try_auto_parse_response:Boolean=True) =
    Data.post self body method headers try_auto_parse_response
-


Looks like a simplification.

JaroslavTulach · 2023-12-20T04:12:48Z

std-bits/base/src/main/java/org/enso/base/enso_cloud/HideableValue.java

+    }
+  }
+
+  String render();


This is usually called toString() in Java.

Fair, I wanted to make sure we call it explicitly to distinguish the usages where we convert secrets to __SECRET__ vs usages where we disallow secrets vs usages where we read actual secret values.

I can change to toString if you think that does not clash with the above.

JaroslavTulach · 2023-12-20T04:14:41Z

std-bits/base/src/main/java/org/enso/base/enso_cloud/HideableValue.java

+    }
+  }
+
+  record PlainValue(String value) implements HideableValue {


Secure computing suggests to store values in byte[] - because (unlike String) byte[] can be programaticaly destroyed. That gives the user of the HideableValue control of the life-span a value is valid.

The PlainValue is nothing that has to be destroyed.

This PR is not changing any of the 'secure' secret logic from #8006, it just refactors it a bit and changes the API.

Also note that for the actual secrets (not something the HideableValue is), we have to hold them as String at some point because we need to pass it to places like JDBC properties.

I think the comment suggests that HideableValue is a misleading name. I really didn't have better ideas how to call this type. Any suggestions?

HideableValue is a type that can either be plain non-sensitive text value OR a secret id that will be resolved into a proper sensitive value much later (the ID itself is nothing that has to be secure).

JaroslavTulach · 2023-12-20T04:20:05Z

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

+ * <p>The query parameters and user info are stored separately, because they may contain secrets and
+ * will only be resolved to plain values within {@link org.enso.base.enso_cloud.EnsoSecretHelper}.
+ */
+public record URIWithSecrets(


How is this different to URISchematic? Why isn't schematic working with HideableValue to beging with?

The URISchematic is just a helper method that I need because I could no longer use the Apache URIBuilder. It is encapsulating parts of the logic of building an URI while overriding some parts of it. I did not want to couple this with the Secrets logic as its completely independent concept.

The URIWithSecrets may be converted into URISchematic in 2 ways - either resolving the secret ids to actual secret values (done only in EnsoSecretHelper) OR resolving secrets to just say __SECRET__ (the 'public' appearance). Then URISchematic takes either of these and builds a proper URI.

jdunkerley · 2023-12-20T17:32:21Z

distribution/lib/Standard/Base/0.0.0-dev/src/Network/URI.enso

+    / : Text -> URI
+    / self segment:Text =


not saying it should, but we could add pre the query string part

I don't think I understand what you mean, could you please clarify?

Isn't what you mean the thing that is checked in this test?

Test.specify "should keep existing query arguments intact when extending the path" <| uri0 = URI.parse "https://example.com/path?a=b&c=d" uri1 = uri0 / "x" / "y" uri2 = uri1 . add_query_argument "e" "f" uri3 = uri2 / "z" uri3.path . should_equal "/path/x/y/z" uri3.query . should_equal "a=b&c=d&e=f" uri3.to_text . should_equal "https://example.com/path/x/y/z?a=b&c=d&e=f"

or is that sth else?

distribution/lib/Standard/Base/0.0.0-dev/src/Network/URI.enso

… it to utils

I have added this in #8591, but I have realised it may not be a good idea to have it, so I am removing that particular change.

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

radeusgd self-assigned this Dec 19, 2023

radeusgd requested review from jdunkerley and GregoryTravis as code owners December 19, 2023 17:39

radeusgd force-pushed the wip/radeusgd/url-encode branch from 734b5e2 to eef7793 Compare December 19, 2023 17:43

radeusgd requested review from 4e6, JaroslavTulach, hubertp, Akirathan and mwu-tow as code owners December 19, 2023 17:43

radeusgd force-pushed the wip/radeusgd/8544-merge-uri-with-query branch from b690744 to 68afb4a Compare December 19, 2023 17:44

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

Merge URI with URI_With_Query #8544

Closed

GregoryTravis approved these changes Dec 19, 2023

View reviewed changes

JaroslavTulach approved these changes Dec 20, 2023

View reviewed changes

radeusgd force-pushed the wip/radeusgd/8544-merge-uri-with-query branch 2 times, most recently from 7d864ed to bf0043e Compare December 20, 2023 09:23

jdunkerley approved these changes Dec 20, 2023

View reviewed changes

Base automatically changed from wip/radeusgd/url-encode to develop December 20, 2023 18:01

enso-bot bot mentioned this pull request Dec 21, 2023

Try to improve performance of Dataflow Errors #8137

Closed

radeusgd added 11 commits December 21, 2023 09:52

remove URI_With_Query from types

fc88c1d

checkpoint

28be69f

report skipped whole groups of tests

d982c3d

WIP

3bfc7e8

WIP...

3b6a0eb

checkpoint

0a92444

Java side of secret resolution, building an URI

1b56436

updating how metadata is fetched to be more robust

1efb0f8

fixes, comaprator, removed old URI_With_Query.enso

489182b

fixing comparator

ca9c0ef

fixes

6e5b01a

radeusgd added 12 commits December 21, 2023 09:53

more edge cases

148205b

tests for user info handling, fixing some edge cases

4d3df47

move test, add edge case

c17f81c

add tests for /, reset_query_arguments, set_fragment

f7bedb5

checkpoint

86bc671

implementing URI transformation - extend path, reset query

4aca9df

fixing edge cases

cccf198

more edge cases!

c64a22a

split test

6c51f9e

docs

ccde4c6

javafmt

0f5e3d6

remove file after rebase

d853c14

radeusgd force-pushed the wip/radeusgd/8544-merge-uri-with-query branch from bf0043e to d853c14 Compare December 21, 2023 08:53

radeusgd added 5 commits December 21, 2023 12:52

Merge branch 'develop' into wip/radeusgd/8544-merge-uri-with-query

2f4566b

CR: newline

065c601

docs

447747c

CR: suggestions - refactor: rename Enso_Secret.to_java_value and move…

4d5b2f6

… it to utils

better split of test groups

b276703

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

mergify bot merged commit d41d48e into develop Dec 21, 2023
31 checks passed

mergify bot deleted the wip/radeusgd/8544-merge-uri-with-query branch December 21, 2023 18:39

radeusgd mentioned this pull request Dec 22, 2023

DataflowError.withoutTrace shall not store a trace #8608

Merged

7 tasks

radeusgd mentioned this pull request Jan 11, 2024

Remove set_user_info from URI #8738

Merged

5 tasks

mergify bot pushed a commit that referenced this pull request Jan 15, 2024

Remove set_user_info from URI (#8738)

5b70ff2

I have added this in #8591, but I have realised it may not be a good idea to have it, so I am removing that particular change.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Merge `URI_With_Query` into `URI`, extend API of `URI` #8591

Merge `URI_With_Query` into `URI`, extend API of `URI` #8591

radeusgd commented Dec 19, 2023

JaroslavTulach left a comment

JaroslavTulach Dec 20, 2023

radeusgd Dec 20, 2023

radeusgd Dec 21, 2023

JaroslavTulach Dec 20, 2023

JaroslavTulach Dec 20, 2023

radeusgd Dec 20, 2023

JaroslavTulach Dec 20, 2023

radeusgd Dec 20, 2023

JaroslavTulach Dec 20, 2023

radeusgd Dec 20, 2023

jdunkerley Dec 20, 2023

radeusgd Dec 21, 2023

radeusgd Dec 21, 2023

Merge URI_With_Query into URI, extend API of URI #8591

Merge URI_With_Query into URI, extend API of URI #8591

Conversation

radeusgd commented Dec 19, 2023

Pull Request Description

Important Notes

Checklist

JaroslavTulach 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

Merge `URI_With_Query` into `URI`, extend API of `URI` #8591

Merge `URI_With_Query` into `URI`, extend API of `URI` #8591