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.

Sign up for GitHub

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: add unique index support on create for in-memory connector #1672

Closed
wants to merge 2 commits into from
Closed

feat: add unique index support on create for in-memory connector #1672

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
6400ebe
feat: add unique index support on create for in-memory connetor
Dec 6, 2018
3342718
fixup! apply feedback
Dec 6, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 15 additions & 0 deletions lib/connectors/memory.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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'));
Copy link
Member

Choose a reason for hiding this comment

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

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];

if (indexes[idx].unique === true) {
for (var instId in this.cache[model]) {
var inst = JSON.parse(this.cache[model][instId]);
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

if (inst[idxPropName] === data[idxPropName]) {
var duplicateIndexError = new Error(g.f('Duplicate entry for %s.%s', model, idxPropName));
duplicateIndexError.statusCode = duplicateIndexError.status = 409;
return fn(duplicateIndexError);
}
}
}
}

Copy link
Contributor

Choose a reason for hiding this comment

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

Please use https://github.com/strongloop/loopback-datasource-juggler/blob/master/lib/model-definition.js#L194

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch!

if (this.collection(model)[id]) {
var error = new Error(g.f('Duplicate entry for %s.%s', model, idName));
error.statusCode = error.status = 409;
Expand Down
18 changes: 18 additions & 0 deletions test/memory.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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});
Copy link
Member

Choose a reason for hiding this comment

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

{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).

ds.automigrate('ProductTest', function(err) {
Copy link
Member

Choose a reason for hiding this comment

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

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.

if (err) return done(err);

Product.create({name: 'a-name'}, function(err, p) {
if (err) return done(err);
Product.create({name: 'a-name'}, function(err) {
should.exist(err);
err.message.should.match(/Duplicate/i);
err.statusCode.should.equal(409);
done();
});
});
});
});

describe('automigrate', function() {
var ds;
beforeEach(function() {
Expand Down