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

Fix (datastore) quote sql commands #712

Merged
merged 6 commits into from
Aug 11, 2020

Conversation

saltonmassally
Copy link
Contributor

@saltonmassally saltonmassally commented Aug 8, 2020

Issue #, if available:
Datastore SQL commands fail if your model has SQLite reserved words in them. see #707

Description of changes:

This quotes table and column names to avoid that.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

Datastore SQL commands fail if your model has SQLite reserved words in them.
This quotes table and column names to avoid that

Resolves: aws-amplify#707
@saltonmassally saltonmassally requested a review from a team August 8, 2020 09:26
@saltonmassally
Copy link
Contributor Author

Cool @saltonmassally! Is the backtick a SQL keyword (SqlKeyword)?

If not -- we also have a utility Wrap in the core module. You could consider to extend this with a Wrap.inBackticks(...) utility method.

It'd be great to add a test case for this, against a real live SQL database. May need to build an instrumentation test under aws-datastore/src/androidTest, for that purpose.

Would love to see this come back as a PR to amplify-android. Thanks @saltonmassally.

Let me do that in a further commit

@saltonmassally
Copy link
Contributor Author

@jamesonwilliams I replaced quote in SqlKeyword with Wrap.inBackticks.

On the subject of having a test case for this, what exactly are we targeting not covered by existing cases? Additionally, I am still finding my way through the code so a pointer to a similar case will be helpful.

@saltonmassally saltonmassally changed the title Fix/quote sql commands Fix (datastore) quote sql commands Aug 9, 2020
@jamesonwilliams
Copy link
Contributor

jamesonwilliams commented Aug 10, 2020

@jamesonwilliams I replaced quote in SqlKeyword with Wrap.inBackticks.

On the subject of having a test case for this, what exactly are we targeting not covered by existing cases? Additionally, I am still finding my way through the code so a pointer to a similar case will be helpful.

The existing tests are passing, so that's a great sign.

We don't currently have any tests that check what you documented in #707. In other words: what happens when a model includes a field that is a SQL reserved word?

The instrumentation tests for the SQL storage layer are in aws-datastore/src/androidTest/java/com/amplifyframework/datastore/storage/sqlite.

The test we want to run is:

  1. Model contains a reserted SQL keyword.
  2. A SQLiteStorageAdapter is initialized, using that model.
  3. Assert: Table creation succeeds
  4. An item is saved into the storage adapter.
  5. Assert: save succeeds
  6. Query for that item.
  7. Assert: query returns original item that you saved in step (4).

There are two strategies to gain this coverage.

  1. Add your own new test file, that does exactly this, referencing some @Model class you create.
  2. Recycle an existing test, by modifying an existing model class. For example, if you look through the existing SQL storage adapter tests, many of them make reference to the BlogOwner in the testmodels module. You could add your SQL-reserved-word-named fields into this model. Then, all of the existing tests would serve as code coverage for your current changes in this PR.

Ping me on Discord if you want to chat more about this! Jameson#3042

Copy link
Contributor

@raphkim raphkim left a comment

Choose a reason for hiding this comment

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

Impressive PR! Thank you for your contribution. I had a few nit-picky comments, but it looks ready to go once you address it.

saltonmassally and others added 2 commits August 12, 2020 01:12
…ore/storage/sqlite/SQLiteStorageReservedWordTest.java

Co-authored-by: Raphael Kim <52714340+raphkim@users.noreply.github.com>
@jamesonwilliams
Copy link
Contributor

LGTM, thanks for the contribution @saltonmassally. You get a "Contributors" badge on Discord, now, as well as cool green lettering for your name. :-D

Copy link
Contributor

@raphkim raphkim left a comment

Choose a reason for hiding this comment

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

LGTM!

@raphkim raphkim merged commit 943e11b into aws-amplify:main Aug 11, 2020
@saltonmassally saltonmassally deleted the fix/QuoteSqlCommands branch October 10, 2020 11:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants