feat(DataStore): Use Int instead of UInt for QueryPaginationInput

[618coffee]

Is your feature request related to a problem? Please describe. Int is the type used mostly and auto inferred by complier, QueryPaginationInput.page(_ page: UInt, limit: UInt) takes in UInt, user need to cast it as Int which is superfluous

Describe the solution you'd like Instead of doing:

var pageNumber = 0
_ = Amplify.DataStore.query(Todo.self, paginate: .page(UInt(pageNumber), limit: 10)) { // handle result }

Anyone should just do

var pageNumber = 0
_ = Amplify.DataStore.query(Todo.self, paginate: .page(pageNumber, limit: 10)) { // handle result }

[royjit]

Added this to milestone 2.0.0 since this will be a breaking change.

[lawmicha]

Is there a way to add the second one without then type compiling issues? like having both .page(Int) and .page(UInt) and then:

QueryPaginationInput.page(10, limit: 10) // does this complain about the ambiguous for type lookup problem?
let value: Int = 10
QueryPaginationInput.page(value, limit: 10) // does this complain about the ambiguous for type lookup problem?
let value2: UInt = 10
QueryPaginationInput.page(value2, limit: 10) // does this complain about the ambiguous for type lookup problem?

[palpatim]

As @lawmicha points out, adding an additional type signature override shouldn't be a breaking change, since this is a static functions, as opposed to enum cases.

    /// Creates a `QueryPaginationInput` in an expressive way, enabling a short
    /// and developer friendly access to an instance of `QueryPaginationInput`.
    ///
    /// - Parameters:
    ///   - page: the page number (starting at 0)
    ///   - limit: the page size (defaults to `QueryPaginationInput.defaultLimit`)
    /// - Returns: a new instance of `QueryPaginationInput`
    public static func page(_ page: Int,
                            limit: Int = QueryPaginationInput.defaultLimit) -> QueryPaginationInput {
        return QueryPaginationInput(page: UInt(page), limit: UInt(limit))
    }

[atierian]

After discussion, we decided that the risks associated with making this change are too great for the minor dx improvement. A couple of points discussed:

This change moves a potential crash from the call site to within Amplify.

var page: UInt = 0
page -= 1 // ❌ crashes here

var page = 0
page -= 1
.page(page) // ❌ crashes in Amplify when casting to UInt

Explicit > implicit (at least in this case). The most likely cause of accidentally assigning a negative value to a page comes from blindly decrementing past 0 based on an external action.

var page = 0
var posts = [Post]()

func nextPageTapped() { 
  page += 1
  fetchPosts()
}

func previousPageTapped() { 
  page -= 1
  fetchPosts()
}

func fetchPosts() { 
  Task { 
    posts = try await Amplify.DataStore.query(
      Post.self,
      paginate: .page(page)
    )
  }
}

By forcing the callsite to explicitly define the type as UInt (var page: UInt8 = 0), we're providing a point to consider the repercussions of blindly decrementing. This additional context disappears when allowing the compiler to infer page to be of type Int.

We explored allowing negative numbers and throwing from the query API. This appears to be the path taken by Android and Flutter (on Android). However, due to the existing...

public struct QueryPaginationInput {
    public static let defaultLimit: UInt = 100
    public let page: UInt
    public let limit: UInt
}

...doing so would be either a breaking change or it would require introducing a new query overload. Neither of which are worth doing considering the very minor benefit this overload introduces. This is an option that we would like to revisit with vNext though.