feat: add unique index support on create for in-memory connector #1672
Conversation
b-admike
requested review from
dhmlau,
jannyHou,
virkt25 and
zbarbuto
as code owners
b-admike
force-pushed
the
feat/memory-connector-index
branch
from
56d5418
to
6400ebe
Compare
| } | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
|
What about |
Good catch. Looks like we don't have that for case of updating PKs as well when there is an existing PK with the same value :-(. I've written the test cases, and will add the logic to handle that use case 👍 |
There was a problem hiding this comment.
Nice start!
According to our docs, it's possible to create a unique index for multiple keys. For example:
"<indexName>": {
"keys": {
"<key1>": 1,
"<key2>": -1
},
"options": {
"unique": true
}
}We need to address this use case too. Either support multi-key indexes, or report a "not implemented" error when such index is encountered.
| @@ -677,6 +677,24 @@ describe('Memory connector', function() { | |||
| }); | |||
| }); | |||
|
|
|||
| it('should refuse to create object with duplicate unique index', function(done) { | |||
| var ds = new DataSource({connector: 'memory'}); | |||
| var Product = ds.define('ProductTest', {name: {type: String, index: {unique: true}}}, {forceId: false}); | |||
There was a problem hiding this comment.
{name: {type: String, index: {unique: true}}}
I see this is an existing & already documented syntax 👍
https://loopback.io/doc/en/lb3/Model-definition-JSON-file.html#indexes
I find this statement as too long & difficult to read when on a single line, could you please split it into multiple lines (e.g. using "one arg per line" rule).
| it('should refuse to create object with duplicate unique index', function(done) { | ||
| var ds = new DataSource({connector: 'memory'}); | ||
| var Product = ds.define('ProductTest', {name: {type: String, index: {unique: true}}}, {forceId: false}); | ||
| ds.automigrate('ProductTest', function(err) { |
There was a problem hiding this comment.
juggler v4 requires Node.js 8+. In new code, we should stop using callbacks and use async/await instead.
In #1671, I am fixing eslint config to allow async functions.
| var idxPropName = idx.substring(0, idx.indexOf('_index')); | ||
| if (indexes[idx].unique === true) { | ||
| for (var instId in this.cache[model]) { | ||
| var inst = JSON.parse(this.cache[model][instId]); |
There was a problem hiding this comment.
This implementation will scale poorly. When there are 1000 records, you have to JSON parse 1000 records to check for uniqueness. That's a lot of CPU work!
IMO, UNIQUE INDEX should be implemented using a Set:
- when creating a new record, add the property value into the Set
- when deleting a record, remove it's property value from the Set
- when updating a record, remove the old property value and add the new property value
Whenever adding a new property value, report an error if the property value already exists in the Set.
There was a problem hiding this comment.
The reason I have to use JSON parse is because we stringify the model instances to store in the cache :(. However, I like that approach and I'm trying it out. I think it will work well with multiple-key unique indexes too.
| @@ -255,6 +255,21 @@ Memory.prototype._createSync = function(model, data, fn) { | |||
| this.collection(model, {}); | |||
| } | |||
|
|
|||
| var indexes = this._models[model].model.definition.indexes(); | |||
| for (var idx in indexes) { | |||
| var idxPropName = idx.substring(0, idx.indexOf('_index')); | |||
There was a problem hiding this comment.
I find this string manipulation hacky and fragile. Let's fix definition.indexes() to copy the property name into index definition.
const idxPropName = indexes[idx].keys[0];
|
This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions. |
|
This issue has been closed due to continued inactivity. Thank you for your understanding. If you believe this to be in error, please contact one of the code owners, listed in the |
Description
Connect to loopbackio/loopback-next#2126. Goal is to add support for unique indexes for
in-memoryconnector. So far I've added logic to throw on duplicate entries for unique index properties. I'm sure there is more to add as I'm not familiar with how the connector works.Related issues
Checklist
guide