Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Integrating Enso Cloud with the libraries (part 1...) #8006

Merged
merged 36 commits into from
Nov 20, 2023

Conversation

jdunkerley
Copy link
Member

@jdunkerley jdunkerley commented Oct 9, 2023

Pull Request Description

  • Add a File_For_Read type. Used for File_Format to read files.
  • Added Enso_User representing the current user in Enso_Cloud.
    • Will be later able to list known users.
  • Added Enso_Secret representing a value defined in Enso_Cloud.
    • Value not used within Enso only accessed within polyglot Java.
    • Integrated into Username_And_Password and can be used within JDBC connections.
    • Integrated into HTTP Headers so a secret can be used as a value.
    • New URI_With_Query with the same API as URI. Supporting secrets in the value.
    • Will be integrated with AWS credentials.
  • Added Enso_File representing a file or a folder in the cloud.
    • Support the same API as File (like the S3_File).
    • Will support enso:// URI style access.

Minor fixes:

  • Linting fixes.
  • Fixed for web APIs:
    • Removed Http_Utils.java and folded functionality into Enso code.
    • Altered old URI API, just returning Nothing instead of throwing as an error.
    • Use HTTP 2 by default.
  • Fix bug in JSON.into creating a Map.
  • Fix bug in S3_File.
  • Added type checking for Vector.+.
  • Added conversion from Column to Vector, allowing the use of columns within Vector methods.
  • Fix allowing using debug mode on Windows.
  • Tweak to Source_Location so that stacktrace renders correctly in IDE.

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.

Comment on lines 44 to 52
## Checks if the folder or file exists
exists : Boolean
exists self =
auth_header = Utils.authorization_header
response = HTTP.fetch self.internal_uri HTTP_Method.Get [auth_header]
response.status_code == 200
Copy link
Member

Choose a reason for hiding this comment

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

If I read this right, we do a full GET request, meaning we actually start downloading the file which may be costly if the file is big + will cost us data transfer.

Can we use HEAD option here instead?

Copy link
Member Author

Choose a reason for hiding this comment

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

In the cloud API, the files GET doesn't give the file it just gives the metadata for the file.
I have asked for a HEAD method to be added to the API to do exists check.

response.if_not_error <|
js_object = response.decode_as_json
assets = js_object.get "assets" []
files = assets.map t-> t:Enso_File
Copy link
Member

@radeusgd radeusgd Nov 7, 2023

Choose a reason for hiding this comment

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

We are using this construct more and more often, I wonder if we should think about adding some support for it, like something similar to assets.cast Enso_File (not sure about the name, as clashing with Table.cast is not ideal).

Maybe assets.ensure_items_type Enso_File.

Not a thing for this PR though, just a general comment about the pattern.

Comment on lines 92 to 93
authorization_basic : Text -> Text -> Header
authorization_basic user pass =
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
authorization_basic : Text -> Text -> Header
authorization_basic user pass =
authorization_basic : Text -> Text|Enso_Secret -> Header
authorization_basic (user : Text) (pass : Text | Enso_Secret) =

Copy link
Member

Choose a reason for hiding this comment

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

Ah ok I see that this would complicate the logic quite significantly.

I think useful to allow this (together with also allowing secrets in authorization_bearer) at some point, but probably not at this PR.

@radeusgd
Copy link
Member

radeusgd commented Nov 7, 2023

I think the overall structure looks great.

Copy link
Member

@radeusgd radeusgd left a comment

Choose a reason for hiding this comment

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

Looks like a great step forward.

I wonder how we can approach integration testing of the cloud parts (but ofc that's for the follow-ups).

.replace("\\b", "\b")
.replace("\\f", "\f")
.replace("\\\\", "\\");
return json.substring(1, json.length() - 1).translateEscapes();
Copy link
Member

Choose a reason for hiding this comment

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

I think it may be good to include a TODO here that JSON can actually contain the Unicode escapes - if we ever run into this being an issue it would be a valuable hint.

js_object = response.decode_as_json
js_object.into Enso_User

## Lists all known users.
Copy link
Contributor

Choose a reason for hiding this comment

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

PRIVATE?

@jdunkerley jdunkerley force-pushed the wip/jd/enso-cloud-api branch 3 times, most recently from e087d9d to 807dfe9 Compare November 17, 2023 11:37
@jdunkerley jdunkerley added CI: Ready to merge This PR is eligible for automatic merge CI: Clean build required CI runners will be cleaned before and after this PR is built. labels Nov 20, 2023
@mergify mergify bot merged commit ecaca12 into develop Nov 20, 2023
29 of 30 checks passed
@mergify mergify bot deleted the wip/jd/enso-cloud-api branch November 20, 2023 23:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CI: Clean build required CI runners will be cleaned before and after this PR is built. CI: Ready to merge This PR is eligible for automatic merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants