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

add missing sql statements and check for existing records #116

Merged
merged 4 commits into from
Nov 19, 2019

Conversation

drochetti
Copy link
Contributor

Notes:

  • refactored the SQL statement creation to be aligned with the GraphQL one. Now both share a common protocol and structure (DataStoreStatement) so each kind of different statement has its own place. This simplifies maintenance and API documentation
  • added missing delete and update SQL statements
  • added exists to StorageEngineAdapter
  • added more tests and documentation

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

**Notes:**

- refactored the SQL statement creation to be aligned with the GraphQL
one. Now both share a common protocol and structure (`DataStoreStatement`)
so each kind of different statement has its own place. This simplifies
maintenance and API documentation
- added missing `delete` and `update` SQL statements
- added `exists` to `StorageEngineAdapter`
- added more tests and documentation
@drochetti drochetti added the datastore Issues related to the DataStore category label Nov 19, 2019
@drochetti drochetti self-assigned this Nov 19, 2019
@drochetti
Copy link
Contributor Author

@palpatim I understand that this PR conflicts with yours a little bit. Let me know if you have any concerns when merging.

The main part of this PR that connects to yours is:

StorageEngineAdapter.exists(_ modelType: Model.Type, withId id: Identifier) throws -> Bool

@@ -0,0 +1,338 @@
//
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This file was just renamed. Only a few test cases here are actually new (not sure why GitHub didn't pick it up as a rename and rendered the proper diff)

**Notes:**

- removed `QueryTranslator` and `Query`
- added tests for more tests to `StorageEngineAdapter`
var statement = "insert into \(schema.name) "
statement += "(\(columns.joined(separator: ", ")))\n"

let variablePlaceholders = Array(repeating: "?", count: columns.count).joined(separator: ", ")
Copy link
Member

Choose a reason for hiding this comment

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

Heaven forbid we ever hit it, but do I remember that SQLite has a limit on the number of bind variables on the order of 1,000? Might be worth verifying that and including it the documentation

Copy link
Contributor Author

Choose a reason for hiding this comment

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

knocks on wood 😅
Thanks for the callout. I created #122 to track it

/// - Returns: a list of columns that can be used in `select` SQL statements
internal func joinedAsSelectedColumns(_ columns: [String], perLine: Int = 3) -> String {
return columns.enumerated().reduce("") { partial, entry in
let spacer = entry.offset == 0 || entry.offset % perLine == 0 ? "\n " : " "
Copy link
Member

Choose a reason for hiding this comment

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

nit: consider using isMultipleOf(_:) in place of == 0 to enhance readability

@drochetti drochetti merged commit 47236b7 into master Nov 19, 2019
@drochetti drochetti deleted the feature/sql-statements branch November 19, 2019 23:52
ameter added a commit that referenced this pull request Mar 7, 2023
ameter added a commit that referenced this pull request Mar 7, 2023
ameter added a commit that referenced this pull request Mar 8, 2023
ameter added a commit that referenced this pull request Mar 8, 2023
harsh62 pushed a commit that referenced this pull request Jul 13, 2023
waleedbutt pushed a commit to hassan31/amplify-swift that referenced this pull request Sep 18, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
datastore Issues related to the DataStore category
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants