Double quote array expansion to prevent splitting by abrassel · Pull Request #69 · unitycatalog/unitycatalog

abrassel · 2024-06-19T05:37:12Z

PR Checklist

A description of the changes is added to the description of this PR.
If there is a related issue, make sure it is linked to this PR.
If you've fixed a bug or added code that should be tested, add tests!
If you've added or modified a feature, documentation in docs is updated

Description of changes

https://github.com/koalaman/shellcheck/wiki/SC2068

Double quotes around $@ (and similarly, ${array[@]}) prevents globbing and word splitting of individual elements, while still expanding to multiple separate arguments.

Testing Changes

bin/uc
Please provide a entity.

Usage: bin/uc <entity> <operation> [options]
Entities: schema, volume, catalog, function, table

By default, the client will connect to UC running locally at http://localhost:8080

To connect to specific UC server, use --server https://<host>

Currently, auth using bearer token is supported. Please specify the token via --auth_token <PAT Token>

For detailed help on entity specific operations, use bin/uc <entity> --help
~/unitycatalog git:[double_quote_array_expansion]
bin/uc schema --help
Please provide a valid sub-command for schema.
Valid sub-commands for schema are: get, create, update, list, delete
For detailed help on schema sub-commands, use bin/uc schema <sub-command> --help
~/unitycatalog git:[double_quote_array_expansion]
bin/uc schema get --help
Usage: bin/uc schema get [options]
Required Params:
  --full_name The full name of the table. The full name is the concatenation of the catalog name, schema name, and table/volume name separated by a dot. For example, catalog_name.schema_name.table_name.
Optional Params:
~/unitycatalog git:[double_quote_array_expansion]
bin/uc schema list
All required parameters are not provided.
Usage: bin/uc schema list [options]
Required Params:
  --catalog The name of the catalog.
Optional Params:
  --max_results The maximum number of results to return.
~/unitycatalog git:[double_quote_array_expansion]
bin/uc catelog list
Invalid entity provided.

Usage: bin/uc <entity> <operation> [options]
Entities: schema, volume, catalog, function, table

By default, the client will connect to UC running locally at http://localhost:8080

To connect to specific UC server, use --server https://<host>

Currently, auth using bearer token is supported. Please specify the token via --auth_token <PAT Token>

For detailed help on entity specific operations, use bin/uc <entity> --help
~/unitycatalog git:[double_quote_array_expansion]
bin/uc catalog list
...

bin/start-uc-server 44
Jul 07, 2024 10:47:24 PM com.fasterxml.jackson.databind.PropertyNamingStrategy$PropertyNamingStrategyBase <init>
WARNING: PropertyNamingStrategy.KebabCaseStrategy is used but it has been deprecated due to risk of deadlock. Consider using PropertyNamingStrategies.KebabCaseStrategy instead. See https://github.com/FasterXML/jackson-databind/issues/2715 for more details.
################################################################### 
#  _    _       _ _            _____      _        _              #
# | |  | |     (_) |          / ____|    | |      | |             #
# | |  | |_ __  _| |_ _   _  | |     __ _| |_ __ _| | ___   __ _  #
# | |  | | '_ \| | __| | | | | |    / _` | __/ _` | |/ _ \ / _` | #
# | |__| | | | | | |_| |_| | | |___| (_| | || (_| | | (_) | (_| | #
#  \____/|_| |_|_|\__|\__, |  \_____\__,_|\__\__,_|_|\___/ \__, | #
#                      __/ |                                __/ | #
#                     |___/              vv0.1.0-SNAPSHOT  |___/  #
###################################################################

#68

tdas · 2024-06-20T15:36:38Z

@ravivj-db can you review this since you added this code?

bin/uc

tdas · 2024-06-28T04:46:49Z

@abrassel @ravivj-db are these changes still needed or relevant?

abrassel · 2024-06-28T05:04:30Z

These are lint fixes. Not super important but intellij will complain about them.

bin/start-uc-server

abrassel · 2024-07-08T05:51:30Z

@creechy - I made the changes. Let me know if you want additional testing!

abrassel · 2024-07-08T08:10:41Z

Hm, looks like tests failed. Will investigate

abrassel · 2024-07-08T20:34:43Z

@creechy heads up - when I run the Tarball Generation Tests on master, I'm also getting a failure.

creechy · 2024-07-09T00:21:01Z

@abrassel

@creechy heads up - when I run the Tarball Generation Tests on master, I'm also getting a failure.

yeah I also get the error on main too, I'll take a closer look.

bin/uc

creechy · 2024-07-09T19:26:21Z

@abrassel

@creechy heads up - when I run the Tarball Generation Tests on master, I'm also getting a failure.

I think you have discovered a latent bug in the tarball test. Creating a catalog with a space in it, like Test Catalog is forbidden. I think the current test is somehow breaking that into two parameters Test Catalog, and the uc client is silently ignoring the extra parameter. Can you change the test code to create the catalog Test_Catalog (change the space to underscore) and see if that resolves the issue?

abrassel · 2024-07-10T01:24:09Z

@abrassel

@creechy heads up - when I run the Tarball Generation Tests on master, I'm also getting a failure.

I think you have discovered a latent bug in the tarball test. Creating a catalog with a space in it, like Test Catalog is forbidden. I think the current test is somehow breaking that into two parameters Test Catalog, and the uc client is silently ignoring the extra parameter. Can you change the test code to create the catalog Test_Catalog (change the space to underscore) and see if that resolves the issue?

I'll give this a try.

Update: Gave it a try and it doesn't quite work. We have some command line arguments for which spaces are valid. I.e.

bin/uc table create --full_name unity.default.myTable --columns "col1 int, col2 double" --storage_location /tmp/uc/myTable

In this case "$@" doesn't properly treat the spaces for col1 int, col2 double.

vikrantpuppala · 2024-07-11T07:00:38Z

tests/test_tarball_generation.py

 # 5. Run and verify CLI
 try:
-    cli_cmd = [os.path.join(bin_dir, "uc"), "catalog", "create", "--name", "Test Catalog"]
+    cli_cmd = [os.path.join(bin_dir, "uc"), "catalog", "create", "--name", "Test_Catalog"]


I don't think we should change this test case. Are commands args with spaces not working?

UC does not allow objects with spaces in their name, so the test with the space in the name should be failing, and now is failing with @abrassel 's double quote changes, which is why I asked him to change this test. I don't know why this test is currently successful before this PR but I believe it should be failing.

creechy · 2024-07-12T17:54:18Z

bin/uc

  # echo "Quoted arguments: $quoted_args_str"
  # echo "Running command: $CLI_JAVA_COMMAND"
-  eval $CLI_JAVA_COMMAND $@  || exit
+  eval "$CLI_JAVA_COMMAND" "$@"  || exit


@abrassel I took a look at why the tutorial tests are failing, I think it's due to this line. Not sure why the eval was used in the first place but I think if you remove it and make the line $CLI_JAVA_COMMAND "$@" || exit that should fix things. Can you give it a try? There might be a way to make the eval work but this now makes sense why all the args are being wrapped in quotes before being passed here.

This seems to also fix #263

vikrantpuppala · 2024-07-22T05:02:55Z

@abrassel could we please make any changes needed to this PR? (please feel free to close if this isn't an issue anymore)

abrassel · 2024-07-23T17:29:20Z

hi @vikrantpuppala , been a bit distracted with some other work lately; I apologize for the delay. Taking a look now

abrassel · 2024-07-23T17:55:54Z

@vikrantpuppala @creechy i think we are good to go.

creechy

lgtm.

creechy · 2024-07-23T20:24:24Z

@haogang this is ready to merge.

closes #39

closes unitycatalog#39

Double quote array expansion to prevent splitting

b5aef18

tdas reviewed Jun 20, 2024

View reviewed changes

bin/uc Outdated Show resolved Hide resolved

creechy reviewed Jul 5, 2024

View reviewed changes

bin/start-uc-server Outdated Show resolved Hide resolved

Add tests for pagination fix

ffbfd81

abrassel requested review from creechy, ravivj-db and tdas July 8, 2024 05:50

more cli script improvements

b202170

fix multi word strings

08c70d4

creechy reviewed Jul 9, 2024

View reviewed changes

bin/uc Outdated Show resolved Hide resolved

creechy reviewed Jul 9, 2024

View reviewed changes

bin/uc Outdated Show resolved Hide resolved

fix multi word strings

d99fd12

vikrantpuppala reviewed Jul 11, 2024

View reviewed changes

creechy reviewed Jul 12, 2024

View reviewed changes

abrassel added 2 commits July 23, 2024 10:36

Merge branch 'main' into double_quote_array_expansion

1b6150a

removed eval

b35ee87

abrassel force-pushed the double_quote_array_expansion branch from f29e173 to b35ee87 Compare July 23, 2024 17:50

abrassel requested review from creechy and vikrantpuppala July 23, 2024 17:55

creechy approved these changes Jul 23, 2024

View reviewed changes

creechy requested a review from haogang July 23, 2024 20:23

haogang approved these changes Jul 24, 2024

View reviewed changes

haogang merged commit e4e20da into unitycatalog:main Jul 24, 2024

abrassel deleted the double_quote_array_expansion branch July 24, 2024 17:35

dennyglee pushed a commit that referenced this pull request Sep 2, 2024

Add ability to edit schema description (#69)

24badda

closes #39

tdas pushed a commit that referenced this pull request Sep 5, 2024

[ui] Add ability to edit schema description (#69)

a398aae

closes #39

rtyler pushed a commit to rtyler/unitycatalog that referenced this pull request Sep 5, 2024

Add ability to edit schema description (unitycatalog#69)

9664982

closes unitycatalog#39

Conversation

abrassel commented Jun 19, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tdas commented Jun 20, 2024

Uh oh!

Uh oh!

tdas commented Jun 28, 2024

Uh oh!

abrassel commented Jun 28, 2024

Uh oh!

Uh oh!

abrassel commented Jul 8, 2024

Uh oh!

abrassel commented Jul 8, 2024

Uh oh!

abrassel commented Jul 8, 2024

Uh oh!

creechy commented Jul 9, 2024

Uh oh!

Uh oh!

Uh oh!

creechy commented Jul 9, 2024

Uh oh!

abrassel commented Jul 10, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

vikrantpuppala Jul 11, 2024

Choose a reason for hiding this comment

Uh oh!

creechy Jul 11, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

creechy Jul 12, 2024

Choose a reason for hiding this comment

Uh oh!

vikrantpuppala Jul 22, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vikrantpuppala commented Jul 22, 2024

Uh oh!

abrassel commented Jul 23, 2024

Uh oh!

abrassel commented Jul 23, 2024

Uh oh!

creechy left a comment

Choose a reason for hiding this comment

Uh oh!

creechy commented Jul 23, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

abrassel commented Jun 19, 2024 •

edited

Loading

abrassel commented Jul 10, 2024 •

edited

Loading

creechy Jul 11, 2024 •

edited

Loading

vikrantpuppala Jul 22, 2024 •

edited

Loading