Query improvements

[fishcharlie]

Closes #796

[github-actions]

Pull Request Test Coverage Report for Build 7604f9714abc921da7729080811d258d45a0658b-PR-798

• 0 of 14 (100.0%) changed or added relevant lines in 1 file are covered.
• No unchanged relevant lines lost coverage.
• Overall coverage remained the same at 100.0%

---

Totals
Change from base Build 17cd52f169c8d14a6b75b1db0aba2640e8143489:	0.0%
Covered Lines:	1237
Relevant Lines:	1237

---

💛 - Coveralls

[MickL]

We need to figure out which comparison types are allowed to be used for range keys and limit it to those.

The comparison operators allowed on the rangeKey can be found here: https://docs.aws.amazon.com/amazondynamodb/latest/developerguide/Query.html#Query.KeyConditionExpressions

The sort key condition must use one of the following comparison operators:

• a = b — true if the attribute a is equal to the value b
• a < b — true if a is less than b
• a <= b — true if a is less than or equal to b
• a > b — true if a is greater than b
• a >= b — true if a is greater than or equal to b
• a BETWEEN b AND c — true if a is greater than or equal to b, and less than or equal to c.

The following function is also supported:

• begins_with (a, substr)— true if the value of attribute a begins with a particular substring.

[MickL]

I tested the PR. A simple query specatator.query('matchId').eq('123).all().exec(); is not working as expected when the schema has an index. In this case it sets IndexName even tho I didnt use .using():

{
      ExpressionAttributeNames:  { '#qha': 'matchId' },
      ExpressionAttributeValues: { ':qhv': { S: '123' } },
      TableName:                 'Match',
      IndexName:                 'matchSpectatorsWatchingIndex',
      KeyConditionExpression:    '#qha = :qhv',
    }

All other operations seem to work correctly now.

[fishcharlie]

I tested the PR. A simple query matchModel.query('matchId').eq('123).all().exec(); is not working as expected when the schema has an index. In this case it sets IndexName even tho I didnt use .using():

{
      ExpressionAttributeNames:  { '#qha': 'matchId' },
      ExpressionAttributeValues: { ':qhv': { S: '123' } },
      TableName:                 'Match',
      IndexName:                 'matchSpectatorsWatchingIndex',
      KeyConditionExpression:    '#qha = :qhv',
    }

All other operations seem to work correctly now.

@MickL Can I see your Schema for this?

[MickL]

This works with specatator.query('matchId').eq('123).all().exec():

const spectatorSchema = new Schema({
  spectatorId: {
    type:     String,
    rangeKey: true,
  },
  matchId:     {
    type:    String,
    hashKey: true,
  },
  watchStart:  Number,
  watchEnd:    Number,
});

This makes a query with IndexName even when not using .using():

const spectatorSchema = new Schema({
  spectatorId: {
    type:     String,
    rangeKey: true,
  },
  matchId:     {
    type:    String,
    hashKey: true,
    index:   {
      name:     'matchSpectatorsWatchingIndex',
      rangeKey: 'watchEnd', // Local secondary index
    },
  },
  watchStart:  Number,
  watchEnd:    Number,
});

[MickL]

Hey @fishcharlie could you make any progress on this yet? :)

[fishcharlie]

Hey @fishcharlie could you make any progress on this yet? :)

@MickL Sadly I have not 😞. I have been super slammed with some personal stuff. This is still on my radar and I'm hoping to look into this more by the end of the weekend at the latest. No guarantees tho.

[fishcharlie]

@MickL Could you please retest this and let me know if I'm missing anything? Also I'd love a PR review here if possible. You have been reading a lot about Query recently and I forget some of the ins and outs.

If you don't feel comfortable with doing a full PR review, I'd really appreciate if you could at least review the tests I created. https://github.com/dynamoosejs/dynamoose/pull/798/files#diff-068619de362833d601fa169c3a9d30cb

In the event you try to filter using contains or attribute_exists on a rangeKey, it will use a filter expression for that rangeKey.

I just want to make sure this covers all the use cases you brought up.

Thanks!!

[hardyscc]

looks great!

[MickL]

@hardyscc How detailed did you review?
@fishcharlie I think for a code review this is a little bit too complicated for me. But I will user-test my use cases (if there only would be a debug option 😜) and I will take time to check all your tests and compare them to the docs. Further I will see if all use cases are covered in the tests. Not sure when I have that time tho.

Maybe you can remove the Draft from the PR?

[MickL]

Is this still up to date?

[fishcharlie]

@MickL Yes it is. Gonna maybe do that in a separate PR. That issue is slightly lower priority compared to the rest of this PR. So I want to get this PR through first then I can come back to that in a cleanup PR.

[hardyscc]

@MickL if the test case cover most of the scenarios, then no need to dig into the actual sources ;)