Custom start sequence and increment amounts

[tscolbe]

I added functionality that I was wanting to use in some of my projects. When auto incrementing a field, there are certain cases where I wanted to start at a number other than one. I find this useful and thought others might as well. Additionally, while I was at it, I added the ability to specify a number to increment by.

I added some tests for this in the test file, and made sure all the other tests still passed.

[coveralls]

Coverage remained the same at 100.0% when pulling 4d0ec77 on tscolbe:develop into 94b4daf on ramiel:develop.

[ramiel]

I finally found some time to review this PR that I really want to merge. I'll put my findings as comments in the code

[tscolbe]

Any updates on this?

[ramiel]

Hello @tscolbe . Yes, I'm actually waiting from you. Any update on the comment I left? Especially on the second one?

I'm very sorry. I wrote a review very long time ago, but I forgot to publish it 🤦‍♂️

[ramiel]

As default value there's nothing wrong on having this as 1, but the user should decide to start it from 0 instead. In MYSQL is allowed and this plugin replicates the AUTO_INCREMENT feature

[ramiel]

This is the only part that must change. The point is this: this library implements a pattern for which the autoincrement value is generated atomically, which means with only ONE operation on DB. This is partially explained here.

If we add a count + findAndModify queries this operation is not atomic anymore. This means that, if another increment is requested after the count result is returned but before the findAndModify is executed, the correctness is not guaranteed anymore.

I understand you need to know if the counter already started or not, but a different solution that uses only one query must be found.

Maybe it can be achieved writing a query similar to

this._counterModel.findOneAndUpdate(
    { id: id, reference_value: referenceValue, seq: {$gte: this._options.start_seq}  },
    {$inc: { seq: incrementAmount},
    {upsert: true, new: true, setDefaultsOnInsert: true}
)

and taking the advantage of the default value if the document is not found. I'm not sure this will work but still, it's essential that we use just ONE query.

[m-ramp]

Tested this out and doesn't seem to work. Looks like if we use the same fields in update and insert setDefaultsOnInsert doesn't work. More details here: Automattic/mongoose#3617

[m-ramp]

An alternative approach would be to initialise the counter model with all default values during startup so that we can safely execute increments queries only.

[ramiel]

This sounds as a good idea. The only drawback is that, when after the counter collection is instantiated, the starting value cannot be changed anymore. Not a big issue, just something to document.
What would be the initial value, start_seq - 1?

[m-ramp]

I think that drawback is something that we could live with for now.
Shouldn't initial value be start_seq - inc_amount ?

EDIT: I just realised that we cant initialise counters for scoped increments where the reference_fields are only known during runtime

[m-ramp]

Side question: just to understand, are you @Pr0Sh00t3r related to @tscolbe?

No, we are not related, was searching for this feature and I found this PR.

The scoped increments are more a pain than a feature.

The scoped increments are why I am using this plugin in the first place, please don't remove it.

And I think i fixed the start_seq even for scoped increments. So the idea is to initialise the counter entry before increment, but not on startup as i suggested before, instead right before the _setNextCounter hook. This may affect performance slightly as there is one more DB call in hook but it actually works.

something like this:

sequence._createCounter(this, (err, seq) => {
                if (err) {
                    callback(err);
                    return;
                }
                if (!_.isNull(seq)) {
                    this[sequence._options.inc_field] = seq;
                    cb();
                } else {
                    sequence._setNextCounter(this, (err, seq) => {
                        if (err) {
                            cb(err);
                            return;
                        }
                        this[sequence._options.inc_field] = seq;
                        cb();
                    });
                }
            });

_createCounter would return the seq number if created, else null and _setNextCounter would work as usual.

@ramiel Do you see any issues with this approach ?

[ramiel]

I'm not really sure to understand what you mean. When is this _createCounter called? If we call when we want to increment, it means we should call it inside _setNextCounter which is called by this in turn... Anyway, the general idea of setting the counter collection on the first increment is good but are we breaking the rule of atomic updates?

[m-ramp]

Well, we could call it inside _setNextCounter itself. Just wrote it as a separate prototype function for the sake of clarity.

   Sequence.prototype._createCounter = function(doc, callback) {
        const id = this.getId();
        const reference_value = this._getCounterReferenceField(doc);
        const startSeq = this._options.start_seq;
        const counterModel = this._counterModel;

        counterModel.findOneAndUpdate(
            {
                id,
                reference_value
            },
            {},
            {
                upsert: true,
                new: true,
                setDefaultsOnInsert: true
            },
            (err, counter) => {
                if (err) return callback(err);
                // upserted is present if entry was created now
                if (_.has(counter, "upserted") && counter.upserted.length > 0)
                    return callback(null, startSeq);
                else return callback(null, null);
            }
        );
    };

This is how I wrote it.

• Creates a new entry if not present
• Does nothing if already present.

but are we breaking the rule of atomic updates?

I only used findOneAndUpdate so I believe it is atomic.

[ramiel]

Yes, the operatios itself is atomic, but not the operation of setting the next counter. Anyway, would you like to propose a PR (or update this one)

[m-ramp]

Will submit a new PR

[MR-omid]

why this pull request is not merged? i really need this feature right now.

[ramiel]

@MR-omid I may understand that you have strict timing to get a feature released but sadly we (me) cannot really fullfill it. You may try to use the branch of this pull request that contains the changes you need with the drawbacks you can read from our comments.

[ramiel]

Included with #70