Fixed bug where the reference_fields were used as id

[linusbrolin]

Even though they are not supposed to be. This caused different schemas to use the same counter, despite having different id's for the counters, but the same reference_fields.

[coveralls]

Coverage remained the same at 100.0% when pulling 2dcf512dd87b701fa984b98aede69295ac82ffe0 on linusbrolin:reference_fields-bug into 5e4247825452bebb75eb64684c6469a1735ace52 on ramiel:master.

[coveralls]

Coverage remained the same at 100.0% when pulling 2dcf512dd87b701fa984b98aede69295ac82ffe0 on linusbrolin:reference_fields-bug into 5e4247825452bebb75eb64684c6469a1735ace52 on ramiel:master.

[coveralls]

Coverage remained the same at 100.0% when pulling 86a5d74077de58e7f3a23de24512cf992399b569 on linusbrolin:reference_fields-bug into 5e4247825452bebb75eb64684c6469a1735ace52 on ramiel:master.

[linusbrolin]

Ok, have no idea what the last error is about... It's not the code, it's the test file.

[ramiel]

Ok, I don't understand very well the problem. By the way, I saw master was very old while I had a ready release. So I did a release (which in turn maybe solve the problem you are facing). Let see if your patch still applies and in case can you create the PR against develop? Thank you as always!

[ramiel]

Ok, in the version which is on master now, after publishing it, the problem should be solved. The reference_field is not used anymore as id to lookup on the counter collection. The proper id is always used. I will add a test for this. Can you tell me if this scenario make sense?

"Two schema use the same reference fields for a sequence. The counter must not be shared between the schema"

[linusbrolin]

Here is a scenario to test against:

var mongooseSequence = require('mongoose-sequence');

export const OfferSchema: SequenceSchema = new Schema({
  company: { type: Schema.Types.ObjectId, ref: 'User', required: true },
  offerNumber: { type: Number }
});

OfferSchema.plugin(mongooseSequence, { id: 'offerNumber_seq', inc_field: 'offerNumber', reference_fields: ['company'] });

export const OrderSchema: SequenceSchema = new Schema({
  company: { type: Schema.Types.ObjectId, ref: 'User', required: true },
  orderNumber: { type: Number }
});

OfferSchema.plugin(mongooseSequence, { id: 'orderNumber_seq', inc_field: 'orderNumber', reference_fields: ['company'] });

Both schemas should have a separate counter, that is also dependent on the company on each schema. Like this: Company#1: offerNumber = 1 orderNumber = 1

Company#2: offerNumber = 1 offerNumber = 2 offerNumber = 3

Company#1: offerNumber = 2 orderNumber = 2

Company#2: offerNumber = 4 orderNumber = 1

[linusbrolin]

I can see in master now that the id is indeed as it should be. However, in _getCounterReferenceField you only collect the values from the reference fields, never the names. This means that if, by chance, two reference fields with different names have the same values, it may have unwanted consequences.

One way to fix that is by replacing this:

reference.push(JSON.stringify(doc[this._options.reference_fields[i]]));

With this:

var obj = {};
obj[this._options.reference_fields[i]] = JSON.stringify(doc[this._options.reference_fields[i]]);
reference.push(obj);

[ramiel]

the _getCounterReferenceField returns the value reference for the counter. This is then incremented looking at id and those values. So, because of the id filtering the problem should never appear. If two reference fields have the same name, the id discriminate them. I'll write a test by the way

[linusbrolin]

Yeah, but what if two counters have specified the same id field but different referencefields? Is it guaranteed that two different reference fields can never ever have the same id though? And by the way, mongoose allows for ids to be something other than an objectid. So globally unique ids are not guaranteed. So in my opinion it is unwise to not save the referencefield names aswell.

[ramiel]

The id unicity (for sequences) is delegated to the developer. What if, on db, there are the different reference fields with the same name too? The problem remains. To prevent possible errors the developer can specify the collection name for counters too. So everything is under developer control and I think adding reference name does not add security. To resume:

• the id is choosen by the developer
• the id unicity is checked through index constraints
• so should be possible to have the same id on the collection for the counters

Is there a case I'm not seeing?

[ramiel]

Sorry, I'm starting to see what you say. Let me add a test to be sure

[ramiel]

PS, I like describing the software constraints through tests as you may have understood

[linusbrolin]

@ramiel Sorry for the delay, yeah I totally understand the need for tests. My boss just doesn't want me to spend a bunch of time on creating tests for every component I use in my projects, so I'll often have to make do without them. But I try to provide tests whenever I can find time for it.

Did you manage to create a test for the scenarios I described here?

[ramiel]

I think your boss is right. You just have to tell it to me and I'll provide :) By the way I had no time to create the test because the only case where the collision can happen is when two different applications run on the same DB and use the same references. It's a bit difficult to put it in a test but I'll work on it.