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

feat(api-graphql): add model index query support #12967

Merged
merged 6 commits into from
Feb 9, 2024

Conversation

iartemiev
Copy link
Contributor

@iartemiev iartemiev commented Feb 6, 2024

Description of changes

Runtime implementation for this feature.

// Given this gen2 schema
const schema = a.schema({
  Post: a
    .model({
      title: a.string().required(),
      description: a.string(),
      viewCount: a.integer(),
      updatedAt: a.string(),
    })
    .secondaryIndexes([ a.index('title').sortKeys(['viewCount']) ])
});

// We can now perform the following index query using Amplify JS:
const { data: posts } = await client.models.Post.listByTitleAndViewCount(
  { title: 'My Post', viewCount: {gt: 4} }
);

Checklist

  • PR description included
  • yarn test passes
  • Tests are changed or added
  • Relevant documentation is changed or added (and PR referenced)

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

Copy link
Contributor

@stocaaro stocaaro left a comment

Choose a reason for hiding this comment

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

I'm torn on this one. There are two lines I'm tempted to hold here.

  1. We can and should decompose functions down into well named, single responsibility private functions to increase the readability.
  2. Casting any into a type gives us no protection and this code is touching the piece that is making promises it isn't guaranteeing.

I'm inclined to hold the line on the 1 and accept that 2 is a bigger issue that deserves individual treatment, but both seem worth discussion / alignment.

const secondaryIdxs = getSecondaryIndexesFromSchemaModel(model);

for (const idx of secondaryIdxs) {
models[name][idx.queryField] = indexQueryFactory(
Copy link
Contributor

Choose a reason for hiding this comment

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

I was surprised TS is cool with this, but it looks like models is any above and specified by the return type here, which doesn't give our customers any type interface protection, does it? How do we get guarantees that all of the typed index methods from the type will be populated at runtime?

stocaaro
stocaaro previously approved these changes Feb 8, 2024
Copy link
Contributor

@stocaaro stocaaro left a comment

Choose a reason for hiding this comment

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

Appreciate the updates. One nit suggestion, but otherwise happy to carry on.

Copy link
Contributor

@jimblanc jimblanc left a comment

Choose a reason for hiding this comment

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

Approving changes to aws-amplify and core.

@iartemiev iartemiev merged commit 0d398a4 into main Feb 9, 2024
31 checks passed
@iartemiev iartemiev deleted the data-client-gsi-support branch February 9, 2024 15:10
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.

None yet

3 participants