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

Jump to bottom

Allow chaining of increment, decrement, and update #2740

Merged
merged 4 commits into from Oct 6, 2018
Merged

Allow chaining of increment, decrement, and update #2740

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
7fa5000
Allow chaining of increment, decrement, and update
Jul 31, 2018
5ddec27
Simplified string
Jul 31, 2018
e8d85e5
Switch back to using + or - in query depending on chain output value
Aug 1, 2018
df721bc
Changes according to PR review
Oct 5, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
48 changes: 37 additions & 11 deletions src/query/builder.js
View file
Open in desktop
Expand Up @@ -806,13 +806,36 @@ assign(Builder.prototype, {
},

// Increments a column's value by the specified amount.
increment(column, amount) {
increment(column, amount = 1) {
if (isObject(column)) {
for (const key in column) {
this._counter(key, column[key]);
}

return this;
}

return this._counter(column, amount);
},

// Decrements a column's value by the specified amount.
decrement(column, amount) {
return this._counter(column, amount, '-');
decrement(column, amount = 1) {
if (isObject(column)) {
for (const key in column) {
this._counter(key, -column[key]);
}

return this;
}

return this._counter(column, -amount);
},

// Clears increments/decrements
clearCounters() {
Copy link

Choose a reason for hiding this comment

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

Could you explain what this endpoint might be used for?
My reaction is that it feels unnecessary

Copy link
Member Author

Choose a reason for hiding this comment

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

It follows the same principals as .clearSelect(), .clearWhere(), .clearOrder().

I've yet to find a real life use-case for these functions myself, but for consistency I added one here as well. Suppose someone has found a use for the other clear-functions, else they wouldn't exist.

Copy link
Contributor

Choose a reason for hiding this comment

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

@wubzz
we use it like this with objection.js for example:

image

this._single.counter = [];

return this;
},

// Sets the values for a `select` query, informing that only the first
Expand Down Expand Up @@ -971,15 +994,18 @@ assign(Builder.prototype, {
// ----------------------------------------------------------------------

// Helper for the incrementing/decrementing queries.
_counter(column, amount, symbol) {
let amt = parseFloat(amount);
if (isNaN(amt)) amt = 1;
this._method = 'counter';
this._single.counter = {
_counter(column, amount) {
amount = parseFloat(amount);

this._method = 'update';

this._single.counter = this._single.counter || [];

this._single.counter.push({
column,
amount: amt,
symbol: symbol || '+',
};
amount,
});

return this;
},

Expand Down
46 changes: 30 additions & 16 deletions src/query/compiler.js
View file
Open in desktop
Expand Up @@ -16,6 +16,7 @@ import {
map,
omitBy,
reduce,
has,
} from 'lodash';
import uuid from 'uuid';

Expand Down Expand Up @@ -538,21 +539,6 @@ assign(QueryCompiler.prototype, {
}
},

// Compile the "counter".
counter() {
const { counter } = this.single;
const toUpdate = {};
toUpdate[counter.column] = this.client.raw(
this.formatter.wrap(counter.column) +
' ' +
(counter.symbol || '+') +
' ' +
counter.amount
);
this.single.update = toUpdate;
return this.update();
},

// On Clause
// ------

Expand Down Expand Up @@ -736,7 +722,35 @@ assign(QueryCompiler.prototype, {
},

// "Preps" the update.
_prepUpdate(data) {
_prepUpdate(data = {}) {
const { counter = [] } = this.single;
const grouped = groupBy(counter, 'column');

for (const column in grouped) {
//Skip?
if (has(data, column)) {
//Needed?
this.client.logger.warn(
Copy link

Choose a reason for hiding this comment

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

Most warns have to do with feature support by different SQL engines;
whereas errors are generally thrown for incorrect query construction. I'd say throw an error.
(just glancing at source/tests)

Copy link
Member Author

Choose a reason for hiding this comment

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

I think its bold to assume calling .increments first and then .update with the same column to be a user mistake, it may just as likely be perfectly intentional.

No strong opinion on what should happen here, but definitely some form of message must be spit out of knex instead of silently ignoring. I opted for warn over error due to the argument above, but not 100% against changing this.

`increment/decrement called for a column that has already been specified in main .update() call. Ignoring increment/decrement and using value from .update() call.`
);
continue;
}

let value = reduce(
grouped[column],
(memo, item) => memo + item.amount,
0
);

const symbol = value < 0 ? '-' : '+';

if (symbol === '-') {
value = -value;
}

data[column] = this.client.raw(`?? ${symbol} ?`, [column, value]);
}

data = omitBy(data, isUndefined);
const vals = [];
const columns = Object.keys(data);
Expand Down