Skip to content

feat(query): add support for composite primary keys in where clause.#590

Merged
kiaking merged 10 commits into
masterfrom
feature/where-composite-keys
Mar 11, 2020
Merged

feat(query): add support for composite primary keys in where clause.#590
kiaking merged 10 commits into
masterfrom
feature/where-composite-keys

Conversation

@cuebit

@cuebit cuebit commented Mar 8, 2020 •
edited
Loading

Copy link
Copy Markdown
Member

Description

This PR adds support for composite primary keys when using whereId and whereIdIn query criteria methods.

Type of PR:

  • Bugfix
  • Feature
  • Enhancement
  • Documentation
  • Other

Breaking changes:

  • No
  • Yes

Details

Query execution with find and findIn support composite primary keys and it seems fitting to provide consistent behaviour for criteria predicates using methods whereId and whereIdIn.

class Subscription extends Model {
  static entity = 'subscriptions'

  static primaryKey = ['id', 'key']

  static fields() {
    return {
      id: this.attr(null),
      key: this.attr('')
    }
  }
}

Subscription.insert([
  { id: 1, key: 'f82e' },
  { id: 2, key: 'c8b9' },
  { id: 3, key: 'ebc6' }
])

Using whereId with composite primary keys:

Subscription.query().whereId([2, 'c8b9']).first()

/*
Subscription { $id: '[2,"c8b9"]', id: 2, key: 'c8b9' }
*/

Using whereIdIn with composite primary keys:

Subscription.query().whereIdIn([[2, 'c8b9'], [3, 'ebc6']]).get()

/*
Subscription { $id: '[2,"c8b9"]', id: 2, key: 'c8b9' },
Subscription { $id: '[3,"ebc6"]', id: 3, key: 'ebc6' }
*/

Maintains ID filters and gracefully cancels ID filters as per normal:

Subscription.query().whereId([2, 'c8b9']).orWhere('id', 3).get()

/*
Subscription { $id: '[2,"c8b9"]', id: 2, key: 'c8b9' },
Subscription { $id: '[3,"ebc6"]', id: 3, key: 'ebc6' }
*/

@cuebit cuebit added the enhancement New feature or request label Mar 8, 2020
@cuebit cuebit self-assigned this Mar 8, 2020
@codecov

codecov Bot commented Mar 8, 2020 •
edited
Loading

Copy link
Copy Markdown

Codecov Report

Merging #590 into master will not change coverage by %.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff            @@
##            master      #590   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           47        47           
  Lines         1888      1904   +16     
  Branches       265       270    +5     
=========================================
+ Hits          1888      1904   +16
Impacted Files Coverage Δ
src/model/Model.ts 100.00% <100.00%> (ø)
src/query/Query.ts 100.00% <100.00%> (ø)

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 08990a8...1fe30c0. Read the comment docs.

@cuebit

cuebit commented Mar 8, 2020

Copy link
Copy Markdown
Member Author

The only addition I would like to consider is error handling.

For example, there is no fishnet for whereId when passing an array, instead it behaves likes whereIdIn.

While this isn’t a major concern, it would make sense for such methods to warn the user or throw an error when passing malformed values. I.e. throwing an error using whereId if a) the entity has no composite key and an array is given, or b) the entity has a composite key and no array is given.

Thoughts?

@kiaking

kiaking commented Mar 8, 2020

Copy link
Copy Markdown
Member

Yea, agree. Since both (a) and (b), it wouldn't work as users would expect, so having error is really nice in this case 👍

@cuebit

cuebit commented Mar 9, 2020

Copy link
Copy Markdown
Member Author

Error handling has been implemented. I've escalated this to both whereId, whereIdIn and find, findIn since both groups share similar behaviour and require the same input validation.

The new method normalizeIndexId is a robust way to handle input validation whilst normalizing indices as it allows for re-usable control over user-provided indexes. This method is strictly limited to dealing with indexes and is pretty self explanatory. It reduces clutter where the same error is to be thrown in multiple places.

In addition, there is a new type guard called PrimaryKey which lends to better typing.

@kiaking let me know if you have any further input or comments.

kiaking
kiaking reviewed Mar 9, 2020
View reviewed changes

@kiaking kiaking left a comment •
edited
Loading

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Just few comment on error messaging! But overall, I really like both normalizeIndexId and PrimaryKey. Seeing all the test and implementation, I don't see any problem here! Wonderful job! 🎉

Comment thread src/query/Query.ts Outdated
Comment thread src/query/Query.ts Outdated
@cuebit

cuebit commented Mar 11, 2020

Copy link
Copy Markdown
Member Author

Latest patch 1fe30c0 fixes #455 – looks like normalizeIndexId has found a good home!

@cuebit cuebit mentioned this pull request Mar 11, 2020
Closed
@cuebit

cuebit commented Mar 11, 2020

Copy link
Copy Markdown
Member Author

@kiaking this PR is stable and ready to merge 👍

@kiaking

kiaking commented Mar 11, 2020

Copy link
Copy Markdown
Member

Niiice! 🎉

@kiaking kiaking merged commit 04909f9 into master Mar 11, 2020
@kiaking kiaking deleted the feature/where-composite-keys branch March 11, 2020 12:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@kiaking kiaking kiaking left review comments

Assignees

@cuebit cuebit

Labels

enhancement New feature or request

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@cuebit @kiaking