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

Embedded document: add auto increment id does not work. #15

Open
Brampage opened this issue Feb 14, 2017 · 13 comments
Open

Embedded document: add auto increment id does not work. #15

Brampage opened this issue Feb 14, 2017 · 13 comments

Comments

@Brampage
Copy link

Brampage commented Feb 14, 2017

The example shows that the auto increment does not work on embedded documents.

This does not work:
The parent document implementation with mongoose-sequence
The embedded document implementation

The parent document's auto increment id does work, however when I add an auto increment id on my embedded document I get this error:
Error: Counter already defined for field "_id" at Sequence.getInstance (C:\Users\\Desktop\Programming\MonDoAPI\node_modules\mongoose-sequence\lib\sequence.js:69:15) at Schema.plugin (C:\Users\...\Desktop\Programming\MonDoAPI\node_modules\mongoose\lib\schema.js:1139:3) at Object.<anonymous> (C:\Users\...\Desktop\Programming\MonDoAPI\models\sensorModule.model.js:21:19) at Module._compile (module.js:570:32) at Object.Module._extensions..js (module.js:579:10) at Module.load (module.js:487:32) at tryModuleLoad (module.js:446:12) at Function.Module._load (module.js:438:3) at Module.require (module.js:497:17) at require (internal/module.js:20:19) at Object.<anonymous> (C:\Users\...\Desktop\Programming\MonDoAPI\controllers\sensorModule.controller.js:1:84) at Module._compile (module.js:570:32) at Object.Module._extensions..js (module.js:579:10) at Module.load (module.js:487:32) at tryModuleLoad (module.js:446:12) at Function.Module._load (module.js:438:3) [nodemon] app crashed - waiting for file changes before starting...

@ramiel
Copy link
Owner

ramiel commented Feb 15, 2017

Hello.
Please, can you try to specify the id in the options of the plugin? Like That:

// for the sensorModule schema
SensorModelSchema.plugin(AutoIncrement, {id: 'sensor_model_id_counter'});

// and for the sensor schema
SensorSchema.plugin(AutoIncrement, {id: 'sensor_id_counter'});

@ramiel
Copy link
Owner

ramiel commented Mar 12, 2017

Any news on this? Did it worked?

@rfns
Copy link

rfns commented Mar 14, 2017

@ramiel

I noticed that you're using a SequenceArchive singleton to verify existing fields. Which means that whenever a new Schema sets the same field as counter, the singleton checks the fields based on all the mongoose defined Schemas.

I also noticed that the method existSequence only checks for the field, not the Schema itself. Maybe if we also check the Schema's name we could fix this issue?

SequenceArchive.prototype.existsSequence = function(id) {
    var seq;
    for (var i = 0, len = this.sequences.length; i < len; i++) {
        seq = this.sequences[i];
        if (_.isEqual(seq.id, id)) // Only checks the associated id
            return true;
    }

    return false;
};

That might trigger the error that @Castrovalva is getting.

@ramiel
Copy link
Owner

ramiel commented Mar 14, 2017

I think this won't fix the problem. If we change the way you say it will avoid the problem from @Castrovalva but then it will increment the same counter for the two different schemas. Maybe I'm wrong or I can't see what you're sayng (easily both) and so if you want to provide a PR with the modification you're welcome.
In any case my idea is to

  • Have the id to be mandatory
    or
  • Generate a better id (if not provided) depending on schema/field

I prefer the first solution as it is robust even for cross-application counters.

@rfns
Copy link

rfns commented Mar 20, 2017

You're right. I reviewed the collection where the counters are being saved and noticed that mongoose-sequence works by using property names only, ignoring the collection/schema's name. This would indeed cause problems for usages other than the OP.

By enforcing a mandatory rule for the id name, wouldn't that generate a breaking change? I guess most of the users are using _id or id as counter for convenience sake.

Maybe I could do a PR to add the schema's name to the counters collection, this follows your second idea.

Now the problem would be how to put that property into an existing counter collection. That might also break the current implementations.

@ramiel
Copy link
Owner

ramiel commented Mar 22, 2017

Yes I know enforcing the id is a breaking change and so the new version would be incompatible with the previous. As you say, considering the schema name can lead to another breaking change. Sadly I think the problem is due to a poor design of mine. I think I'll go for the mandatory id solution because it is easier and it has the same guarantees than the other. And, as a bonus point, it will not break any implementation which already set the id. If you think we can manage adding the schema name to the counter without breaking any implementation, please send your PR, it would be awesome!

@rfns
Copy link

rfns commented Apr 10, 2017

Enforcing the name ids would not cause any database breaking change (only API), however it would keep the rubbish counters.

Uhm... what if when boostraping the plugin it could fetch the highest ids from each plugin enabled schema and assume that this would be the correct one to reassign to the Counters collection? I mean, counters are incremental only right? So the plugin shouldn't care about the lower ids, neither the database nor the application.

So maybe it could do something like that:

  1. Checks if the current schema is already up-to-date with the new format.
  2. If not, findOne with sort({ _id: -1 }) for each plugin enabled schema.
  3. Stores a reference to this collection and it's id (counter).
  4. After all references are stored, checks the Counter collection for the conflicting ids.
  5. If there's a conflict then updates the counter with n + 1, where n = current schema id + index of conflicting schema (that's to assure the plugin wont use another matching id).
  6. Adds the schema or colllection name property using the stored reference name and updates the counter related to the schema.

The reference could be something like this:

{ name: "Collection", counter: 2382732 }

I'm gonna fork this project and start implementing this idea after your thoughts.

Ahh, btw this might works for both approaches.

@vamshi9666
Copy link

i am still facing ths issue . any solution ?

@ramiel
Copy link
Owner

ramiel commented May 5, 2018

If you specify the Id for the counter you should have no problem as said in other comments.

@darsh-sn
Copy link

It would be great if someone posts solution for this

@OrlandoRibera
Copy link

For someone with the same problem, and who did not understand the answer as I did in the beginning, this was my solution:

My code with the error:


const commentsSchema = new Schema({
    _id: Number,
   some...
});
commentsSchema.plugin(AutoIncrement);

module.exports = mongoose.model("Comments", commentsSchema);


and it was fixed by changing the autoincrement part to this:
commentsSchema.plugin(AutoIncrement, {id: '<autoincrement_name>', inc_field: '_id'});

for example:
commentsSchema.plugin(AutoIncrement, {id: 'comments_id_counter', inc_field: '_id'});

@neilthawani
Copy link

For someone with the same problem, and who did not understand the answer as I did in the beginning, this was my solution:

My code with the error:


const commentsSchema = new Schema({
    _id: Number,
   some...
});
commentsSchema.plugin(AutoIncrement);

module.exports = mongoose.model("Comments", commentsSchema);

and it was fixed by changing the autoincrement part to this:
commentsSchema.plugin(AutoIncrement, {id: '<autoincrement_name>', inc_field: '_id'});

for example:
commentsSchema.plugin(AutoIncrement, {id: 'comments_id_counter', inc_field: '_id'});

This resolved my issue.

@Facundojimenez
Copy link

Thanks, that solved my problem!

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

No branches or pull requests

8 participants