Add Filtering with QueryParameters #26
Conversation
EnriqueL8
changed the title
Codecov Report
@@ Coverage Diff @@
## master #26 +/- ##
==========================================
+ Coverage 21.04% 21.36% +0.31%
==========================================
Files 6 6
Lines 1259 1512 +253
==========================================
+ Hits 265 323 +58
- Misses 994 1189 +195
Continue to review full report at Codecov.
|
|
Instead of |
|
Do we have an equivalent for delete, e.g.: Model.deleteAll(matching: MyQuery) { error in
...
} |
EnriqueL8
force-pushed
the
query_parameters
branch
from
f7c3dff to
ea6fdf4
Compare
Sources/SwiftKueryORM/Model.swift
Outdated
| do { | ||
| queryDictionary = try QueryEncoder().encode(queryParams) | ||
| } catch { | ||
| throw error |
There was a problem hiding this comment.
If you're just going to rethrow the error you don't need the do{} at all?
Sources/SwiftKueryORM/Model.swift
Outdated
| throw RequestError(.ormQueryError, reason: "Could not extract values for Query Parameters") | ||
| } | ||
|
|
||
| var filter: Filter! = nil |
There was a problem hiding this comment.
This section is deeply mysterious...
There was a problem hiding this comment.
I'll add a comment to explain
| var dictionariesTitleToValue = [[String: Any?]]() | ||
|
|
||
| connection.connect { error in | ||
| if let error = error { |
There was a problem hiding this comment.
Could this be a guard to reduce the indentation level below?
There was a problem hiding this comment.
Could be done, also in another refactoring PR?
There was a problem hiding this comment.
Not sure about the guard (sadly, I don't think you can guard and bind variable inside the block, only outside?) But the else looks redundant given the block returns
| /// - Parameter matching: Optional QueryParams to use | ||
| /// - Returns: An array of model | ||
| static func findAll<Q: QueryParams>(using db: Database? = nil, matching queryParams: Q, _ onCompletion: @escaping ([Self]?, RequestError?) -> Void) { | ||
| guard let database = db ?? Database.default else { |
There was a problem hiding this comment.
"Getting a connection" is so common that maybe it should be factored out into a common function?
There was a problem hiding this comment.
This can be done in another refactoring PR?
EnriqueL8
force-pushed
the
query_parameters
branch
from
ea6fdf4 to
02ac0d1
Compare
|
This should be good to go? @ianpartridge @seabaylea |
| var queryDictionary: [String: String] = try QueryEncoder().encode(queryParams) | ||
|
|
||
| var columns = table.columns.filter { queryDictionary[$0.name] != nil } | ||
| var values = Array(queryDictionary.values) |
There was a problem hiding this comment.
Might want to check that columns and values line up if a user can specify an incorrect column name (one that is not in the table.columns) since the filter would drop that column but the corresponding value would not be dropped.
There was a problem hiding this comment.
Very true, will make that check
| var filter: Filter = (columns.removeFirst() == values.removeFirst()) | ||
| for (column, value) in zip(columns, values) { | ||
| filter = filter && (column == value) | ||
| } |
There was a problem hiding this comment.
Is this way of constructing the filter vulnerable to SQL injection (cf https://github.com/IBM-Swift/Swift-Kuery#sql-injection-prevention-using-parameterization)?
There was a problem hiding this comment.
This issue is pending... #19 but yes that should have been looked at in this PR
This PR enables filtering in the ORM. It uses the Query Parameters from Kitura.
By creating a struct as such:
And calling the api as such: