-
Notifications
You must be signed in to change notification settings - Fork 326
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
Save Database connection as data link, SQL Server data link support #11343
Merged
mergify
merged 15 commits into
develop
from
wip/radeusgd/11294-save-as-datalink-snowflake-sqlserver
Oct 17, 2024
Merged
Changes from all commits
Commits
Show all changes
15 commits
Select commit
Hold shift + click to select a range
1c8c801
checkpoint
radeusgd ef115b5
extract common code for datalinks
radeusgd c955829
better display text
radeusgd a96a090
indent
radeusgd aeea20e
saving Snowflake datalink
radeusgd 889c53a
saving SQLServer datalink (WIP)
radeusgd 111033d
fixes
radeusgd d5139ff
extract common saving datalink tests
radeusgd 927f312
enable save tests in Snowflake
radeusgd c715c1b
add schema and test
radeusgd 02973e0
add SQLServer datalink logic and tests
radeusgd 4000354
fix a test
radeusgd 1c517f7
add space
radeusgd 88f0f22
always allow limit as its fundamental (to be revised if needed)
radeusgd 7f85fdb
fix test: column types are not relevant for it, drop table should use…
radeusgd File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
47 changes: 47 additions & 0 deletions
47
distribution/lib/Standard/Database/0.0.0-dev/src/Internal/Data_Link_Setup.enso
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,47 @@ | ||
from Standard.Base import all | ||
import Standard.Base.Enso_Cloud.Data_Link.Data_Link | ||
import Standard.Base.Errors.File_Error.File_Error | ||
import Standard.Base.Errors.Illegal_Argument.Illegal_Argument | ||
import Standard.Base.Errors.Illegal_State.Illegal_State | ||
import Standard.Base.Runtime.Context | ||
from Standard.Base.Enso_Cloud.Data_Link_Helpers import data_link_extension, secure_value_to_json, save_password_for_data_link | ||
|
||
import project.Connection.Credentials.Credentials | ||
|
||
## PRIVATE | ||
type Data_Link_Setup | ||
## PRIVATE | ||
Available create_data_link_structure:Enso_File->JS_Object | ||
|
||
## PRIVATE | ||
Unavailable cause:Text | ||
|
||
## PRIVATE | ||
Returns an unavailable setup with reason being the connection was alraedy a data link. | ||
already_a_data_link -> Data_Link_Setup = Data_Link_Setup.Unavailable "Saving connections established through a Data Link is not allowed. Please copy the Data Link instead." | ||
|
||
## PRIVATE | ||
save_as_data_link self destination on_existing_file:Existing_File_Behavior = case self of | ||
Data_Link_Setup.Available create_fn -> Context.Output.if_enabled disabled_message="As writing is disabled, cannot save to a Data Link. Press the Write button ▶ to perform the operation." panic=False <| | ||
case destination of | ||
_ : Enso_File -> | ||
replace_existing = case on_existing_file of | ||
Existing_File_Behavior.Overwrite -> True | ||
Existing_File_Behavior.Error -> False | ||
_ -> Error.throw (Illegal_Argument.Error "Invalid value for `on_existing_file` parameter, only `Overwrite` and `Error` are supported here.") | ||
exists_checked = if replace_existing.not && destination.exists then Error.throw (File_Error.Already_Exists destination) | ||
exists_checked.if_not_error <| | ||
json = create_fn destination | ||
Data_Link.write_config destination json replace_existing | ||
_ -> Error.throw (Illegal_Argument.Error "Currently a connection can only be saved as a Data Link into the Enso Cloud. Please provide an `Enso_File` as destination.") | ||
|
||
Data_Link_Setup.Unavailable cause -> | ||
Error.throw (Illegal_Argument.Error "Cannot save connection as Data Link: "+cause) | ||
|
||
## PRIVATE | ||
save_credentials_for_data_link data_link_location:Enso_File credentials:Credentials -> JS_Object = | ||
# A plain text is automatically promoted to a secret. | ||
secret_password = save_password_for_data_link data_link_location credentials.password | ||
|
||
# But we keep the username as-is - if it was in plain text, it will stay in plain text. | ||
JS_Object.from_pairs [["username", secure_value_to_json credentials.username], ["password", secure_value_to_json secret_password]] |
70 changes: 0 additions & 70 deletions
70
...ution/lib/Standard/Database/0.0.0-dev/src/Internal/Postgres/Postgres_Data_Link_Setup.enso
This file was deleted.
Oops, something went wrong.
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is this check removed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was getting failures like:
when invoking
DB_Table.read
.I'm surprised it was working at all in the first place, because our
read
by default relies onlimit
. It is actually an interesting thing to investigate.But overall - as currently implemented -
limit
is an intrinsic part of theread
operation, so it must be supported if we want properread
. Thus I don't think it should be hidden behindSample
flag. We could hide it if we change howread
works, but I'm not convinced we should.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay, so I figured it out.
The tests that were currently being ran for SQLServer just never called into
DB_Table.read
directly... Most of the tests would dotable.at "col" . to_vector
. This calls read but with parameter..All_Rows
meaning thelimit
is skipped.But if I just call
read
with default parameters,limit
is needed to make it work. IMO this makeslimit
fundamental to the basic read capability. We can redo read to work differently if we want to, but for now I think it is OK to do this. Also implementing limit is not very complicated for new backends, so I think it should be fine-ish.