Add more options in findAll #38

AnthonyAmanse · 2018-05-30T19:24:08Z

This commit adds in options:

ORDER BY clause for sorting
OFFSET and LIMIT for retrieving a portion of the rows

This builds the appropriate SQL queries for the options listed above.
This commit also adds in initial tests for the new options.

Signed-off-by: AnthonyAmanse [email protected]

CLAassistant · 2018-05-30T19:24:15Z

Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you all sign our Contributor License Agreement before we can accept your contribution.
0 out of 2 committers have signed the CLA.

❌ djones6
❌ AnthonyAmanse
_{You have signed the CLA already but the status is still pending? Let us recheck it.}

codecov-io · 2018-05-30T19:27:31Z

Codecov Report

Merging #38 into master will increase coverage by 0.04%.
The diff coverage is 33.33%.

@@            Coverage Diff            @@
##           master     #38      +/-   ##
=========================================
+ Coverage   30.05%   30.1%   +0.04%     
=========================================
  Files           6       6              
  Lines        1364    1385      +21     
=========================================
+ Hits          410     417       +7     
- Misses        954     968      +14

Flag	Coverage Δ
#SwiftKueryORM	`30.1% <33.33%> (+0.04%)`	⬆️

Impacted Files	Coverage Δ
Sources/SwiftKueryORM/Model.swift	`38.36% <33.33%> (-0.15%)`	⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 9d9618d...3c72ff7. Read the comment docs.

EnriqueL8 · 2018-05-31T12:46:49Z

Hey @AnthonyAmanse, this looks really good! Just a few comments:

We have some work planned around QueryParams and filtering so it would be good to just have these options in the findAll calls without QueryParams
Some verification needs to take place with the OrderBy parameter to prevent SQL Injection - for example verifying that the column is actually in the table? or using the Parameter() function from SwiftKuery

Thank You

EnriqueL8 · 2018-05-31T14:05:25Z

One more suggestion: would be to make the orderBy parameter accept multiple OrderBy instance since for example we would want to order by ascending name and descending age. The definition could be: orderBy: OrderBy? ...

AnthonyAmanse

@EnriqueL8 Thanks for the feedback. Updated the function to accept a variadic parameter of orderBy. Not sure what to do with verification for columns though

AnthonyAmanse · 2018-06-22T19:08:36Z

Tests/SwiftKueryORMTests/TestFind.swift

+        var column: Column?
+        do {
+            let table = try Person.getTable()
+            column = table.columns.first(where: {$0.name == "age"})


@EnriqueL8 Would this be enough for a verification if the column exists? Since the orderBy parameter is an OrderBy buildable that doesn't accept just any string, verification would have to be done outside of findAll function

Annoyingly yes - the OrderBy only accepts a Field type that can be for example a Column but also a RawField.
We could use Order that accepts a String and then find a Column matching that String and then create the OrderBy case.

EnriqueL8 · 2018-06-25T08:26:12Z

Tests/SwiftKueryORMTests/TestFind.swift

+            XCTAssertNil(error, "Find Failed: \(String(describing: error))")
+        }
+        performTest(asyncTasks: { expectation in
+            Person.findAll(orderBy: OrderBy.DESC(column!)) { array, error in


This need to be .ASC

EnriqueL8 · 2018-06-26T08:33:45Z

@AnthonyAmanse Sorry - this needs now a rebase since your Swift-Kuery 2.0.0 PR just got merged and addressing the comments

AnthonyAmanse · 2018-07-03T04:24:18Z

@EnriqueL8 No worries. Changes made, used Order instead of OrderBy in functions. Also added a test case if column is not found in table

EnriqueL8 · 2018-07-03T08:49:27Z

Sources/SwiftKueryORM/Model.swift

-    let query = Select(from: table)
+    var query = Select(from: table)
+    var orderByArray: [OrderBy] = []
+    for order in order {


Could you extract this logic into a function?

AnthonyAmanse · 2018-07-03T18:51:15Z

@EnriqueL8 Moved the logic to getOrderBy

EnriqueL8 · 2018-07-04T09:40:53Z

Sources/SwiftKueryORM/Model.swift

+  /**
+    This function constructs an array of OrderBy from the Order values
+  */
+  private static func getOrderBy(order: [Order], table: Table) throws -> [OrderBy] {


This method looks quite familiar to the above method, maybe we could call this method from the above to remove code duplication

EnriqueL8 · 2018-07-04T09:43:04Z

Sources/SwiftKueryORM/Model.swift

+    var orderByArray: [OrderBy] = []
+    for order in order {
+      guard let column = table.columns.first(where: {$0.name == order.value}) else {
+        throw RequestError(.ormInternalError, reason: "Column \(order.value) not found")


Instead of throwing an .ormInternalError, we should throw .ormQueryError because it failed during the construction of the Query?

EnriqueL8 · 2018-07-04T09:44:03Z

Sources/SwiftKueryORM/Model.swift

+      guard let column = table.columns.first(where: {$0.name == order.value}) else {
+        throw RequestError(.ormInternalError, reason: "Column \(order.value) not found")
+      }
+      if case .asc(order.value) = order {


Can we remove the order.value? And just have .asc(_)?

EnriqueL8 · 2018-07-04T10:11:12Z

Tests/SwiftKueryORMTests/CommonUtils.swift

        self.numberOfRows = numberOfRows
+        if let sortedByAge = sortedByAge {


Could we sort the array programatically? By using array.sort? In the case of having to change the rows we would also have to update these two lines.

EnriqueL8

Sorry - I promise these are the last changes that I suggest!

AnthonyAmanse · 2018-07-13T15:50:06Z

@EnriqueL8 Feedbacks are always great! Merged the getOrderBy function instead of overloading. Sorted the rows using rows.sort instead of hardcoding it.

EnriqueL8 · 2018-07-18T08:43:02Z

LGTM - Updated Branch because it is out-of-date
Needs a Squash

This commit adds in options: * ORDER BY clause for sorting * OFFSET and LIMIT for retrieving a portion of the rows This builds the appropriate SQL queries for the options listed above. This commit also adds in initial tests for the new options. Signed-off-by: AnthonyAmanse <[email protected]>

AnthonyAmanse force-pushed the additional-options branch 2 times, most recently from 65a9655 to 3c72ff7 Compare June 22, 2018 18:43

AnthonyAmanse commented Jun 22, 2018

View reviewed changes

EnriqueL8 reviewed Jun 25, 2018

View reviewed changes

AnthonyAmanse force-pushed the additional-options branch from 3c72ff7 to ff27ca9 Compare July 3, 2018 03:37

EnriqueL8 reviewed Jul 3, 2018

View reviewed changes

AnthonyAmanse force-pushed the additional-options branch from ff27ca9 to 3a898b7 Compare July 3, 2018 17:39

EnriqueL8 reviewed Jul 4, 2018

View reviewed changes

AnthonyAmanse force-pushed the additional-options branch from 3a898b7 to a03ea46 Compare July 13, 2018 04:51

EnriqueL8 approved these changes Jul 18, 2018

View reviewed changes

AnthonyAmanse force-pushed the additional-options branch from 2ea13b7 to f285773 Compare September 6, 2018 18:45

Merge branch 'master' into additional-options

2cb1a0d

kilnerm self-requested a review January 7, 2019 10:03

kilnerm added the enhancement New feature or request label Jan 7, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add more options in findAll #38

Add more options in findAll #38

AnthonyAmanse commented May 30, 2018

CLAassistant commented May 30, 2018 •

edited

Loading

codecov-io commented May 30, 2018 •

edited

Loading

EnriqueL8 commented May 31, 2018 •

edited

Loading

EnriqueL8 commented May 31, 2018

AnthonyAmanse left a comment

AnthonyAmanse Jun 22, 2018

EnriqueL8 Jun 25, 2018

EnriqueL8 Jun 25, 2018

EnriqueL8 commented Jun 26, 2018

AnthonyAmanse commented Jul 3, 2018

EnriqueL8 Jul 3, 2018

AnthonyAmanse commented Jul 3, 2018

EnriqueL8 Jul 4, 2018

EnriqueL8 Jul 4, 2018 •

edited

Loading

EnriqueL8 Jul 4, 2018

EnriqueL8 Jul 4, 2018

EnriqueL8 left a comment

AnthonyAmanse commented Jul 13, 2018

EnriqueL8 commented Jul 18, 2018 •

edited

Loading

		self.numberOfRows = numberOfRows
		if let sortedByAge = sortedByAge {

Add more options in findAll #38

Are you sure you want to change the base?

Add more options in findAll #38

Conversation

AnthonyAmanse commented May 30, 2018

CLAassistant commented May 30, 2018 • edited Loading

codecov-io commented May 30, 2018 • edited Loading

Codecov Report

EnriqueL8 commented May 31, 2018 • edited Loading

EnriqueL8 commented May 31, 2018

AnthonyAmanse left a comment

Choose a reason for hiding this comment

AnthonyAmanse Jun 22, 2018

Choose a reason for hiding this comment

EnriqueL8 Jun 25, 2018

Choose a reason for hiding this comment

EnriqueL8 Jun 25, 2018

Choose a reason for hiding this comment

EnriqueL8 commented Jun 26, 2018

AnthonyAmanse commented Jul 3, 2018

EnriqueL8 Jul 3, 2018

Choose a reason for hiding this comment

AnthonyAmanse commented Jul 3, 2018

EnriqueL8 Jul 4, 2018

Choose a reason for hiding this comment

EnriqueL8 Jul 4, 2018 • edited Loading

Choose a reason for hiding this comment

EnriqueL8 Jul 4, 2018

Choose a reason for hiding this comment

EnriqueL8 Jul 4, 2018

Choose a reason for hiding this comment

EnriqueL8 left a comment

Choose a reason for hiding this comment

AnthonyAmanse commented Jul 13, 2018

EnriqueL8 commented Jul 18, 2018 • edited Loading

CLAassistant commented May 30, 2018 •

edited

Loading

codecov-io commented May 30, 2018 •

edited

Loading

EnriqueL8 commented May 31, 2018 •

edited

Loading

EnriqueL8 Jul 4, 2018 •

edited

Loading

EnriqueL8 commented Jul 18, 2018 •

edited

Loading