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

Checks for an empty, undefined or null object on transacting #2494

Merged
merged 2 commits into from Feb 20, 2018

Conversation

Projects
None yet
5 participants
@nicosommi
Contributor

nicosommi commented Feb 19, 2018

Hi, description below. Tests included.
Please advice any further modification you consider

Fixes #2079

Those values indicates that the developer probably made a mistake.

nicosommi added some commits Feb 19, 2018

Checks for an empty, undefined or null object on transacting
Fixes #2079

Those values indicates that the developer probably made a mistake.
@elhigu

This comment has been minimized.

Collaborator

elhigu commented Feb 20, 2018

Thanks! Looks good

@elhigu elhigu merged commit 522ccba into tgriesser:master Feb 20, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@Sammons

This comment has been minimized.

Sammons commented Apr 19, 2018

This is a majorly breaking change, since previously you could write functions that optionally participated in a larger transaction. For lots of select calls you may intentionally call a repository method without a transaction in order to capture the pre-transaction state of something.

e.g.

knex.transaction(async transaction => {
  await repo.update(....do something to the person...., transaction);
  // do some logic, oh nevermind, set it back
  const originalValue= await repo.lookup(...get the person, null/* no transaction, capture the outside version of the person */);
  // etc. set it back.
  // proceed with other transactional calls.
})

Another pattern, for code re-use is to do

// in a more complex controller endpoint, you want the same consistent snapshot
knex.transaction(async transaction => {
  const data1 = await repo1.lookup(...params..., transaction);
  const data2 = await  repo2.lookup(...params..., transaction);
  return;
});

// in a simple endpoint, doesn't even need a transaction
await repo1.lookup(...params..., null);
@nicosommi

This comment has been minimized.

Contributor

nicosommi commented Apr 20, 2018

@Sammons if you don't have a transaction you can just omit the call to transacting, I don't see any issue there... why calling transacting if you don't want the query to use the transaction?

Can you put a knex specific example? (lookup method is not in the docs and I don't know what repo1/repo2 is)

@wubzz

This comment has been minimized.

Collaborator

wubzz commented Apr 20, 2018

The argument "dont call transacting unless you have a transaction object" is valid and that is how I changed my own projects using knex to reflect these changes.

However, it is still a very breaking change in the sense that existing code becomes completely unusable and requires changes, especially so since this throws at runtime and may not be too easy to cover. So it's kinda unfortunate this was released in a patch.

At the end of the day I personally welcome this change for consistency. I don't see calling null/undefined/etc as "potential developer mistake" (which moreover shouldn't be knex's responsibility to cover to begin with) but rather as a way to force clarity which imo is always nice.

@Sammons

This comment has been minimized.

Sammons commented Apr 20, 2018

@wubzz right, the problem is the scale of the project. Simply going and modifying what is at this point hundreds, maybe more, of functions across dozens of modules is not really pragmatic. It also forces us to add an if statement to previously linear code. So now to retain good code coverage we have to write more tests. You could say we should have been doing that before.... but it didn't happen. Practically every query we've written is affected by this change - to the point it was a pattern we chose to follow.

@nicosommi

This comment has been minimized.

Contributor

nicosommi commented Apr 20, 2018

@wubzz got it. I think you're partially right. It is true that this may break your current implementation. But that's actually intended, IMO. Knex does not have the responsibility on the developer mistake, but it needs a good API, and if your API leads you to bad usage, then is a bad API. We can discuss and change the message details, the chosen words, etc.

@Sammons true, adding an if will add a new branch to your coverage to test. However, if you in-distinctively called the same function with and without transaction object, and you did it with 100% test code coverage, then you have that test ready on the scenario in which the transaction has a value and on the scenario in which transaction does not have a value.

However I agree with both, technically speaking, that this may be considered a breaking change and left to a minor version change instead of a patch. In my case I will switch to that new version immediately and put a notice on the readme about that issue for the 0.14 version.

@Sammons

This comment has been minimized.

Sammons commented Apr 20, 2018

@nicosommi Thank you. To be clear I'm just trying to emphasize that it's a lot of work to migrate, at least for us - even if the reasons why we have to do the work seem silly.

So there's no hope of rolling back this change in a future version? I understand the desire for consistency but it seems like a minor reason to introduce a breaking change, on something that appears to have had the same behavior for a very long time.

@elhigu

This comment has been minimized.

Collaborator

elhigu commented Apr 20, 2018

Reason for this kind of changes is to prevent accidental ’something was undefined by accident’ errors, which are many times really hard to track. We did this already before for .where clauses which were ignoring clause completely fron query if passed value was undefined.

There is no chance that this would be reverted, but I’am open for configuration switch would allow users to select not throwing errors, which could be deprecated some day maybe.

The reason why this hadn’t been considered breaking change was because passing undefined/null to transacting has been undefined behaviour earlier and code relaying to undefined behaviour .... well keeps workin undefined ways.

This discussion about breaking change seems to raise quite often, when we change some undefined behaviour to defined one.

@elhigu

This comment has been minimized.

Collaborator

elhigu commented Apr 20, 2018

Btw. this feature seems to be still undefined, since this PR didn’t fix documentation to tell about accepted parameters (at least I suppose there is no mention about accepted parameters... im too lazy to actually check docs on mobile)

@Sammons

This comment has been minimized.

Sammons commented Apr 20, 2018

@elhigu if I update the docs and add a configuration option, how would that sound? We already released the breaking change so the configuration should default to requiring the transaction to exist.

@elhigu

This comment has been minimized.

Collaborator

elhigu commented Apr 22, 2018

@Sammons that sounds good. It is definitely a good thing that parameters are verified to be valid by default.

@Sammons

This comment has been minimized.

Sammons commented Apr 23, 2018

@elhigu @nicosommi - What the approach would normally be for this config? Would this be stored in an environment variable, or should it be on the config object?

@elhigu

This comment has been minimized.

Collaborator

elhigu commented Apr 25, 2018

New property in configuration object. The same way with useNullAsDefault.

taneliang added a commit to nusmodifications/nusmods that referenced this pull request Apr 29, 2018

Revert Knex to 0.14.4 to fix api/data tests
Knex PR tgriesser/knex#2494, released in Knex
0.14.5, is causing a new test error.
@StreetStrider

This comment has been minimized.

StreetStrider commented Jun 2, 2018

You wont believe me, but I've used passing empty value as a feature, to indicate that this query is not required to be a part of transaction.

@nicosommi

you can just omit the call to transacting

That isn't nice in chaining API.
before:

.select(…)
.transacting(tx) // `tx` may be empty value
.where(…)

now:

var q =select(…)
tx && q.transacting(tx)
q.where(…)

Does knex has any of thru, pipe or noop?
(something like thru(q => tx && q.transacting(tx) || q) may suit for chaining API).

However, explicit is better than implicit, so I will mitigate the aftermaths of my side. But you definitely should have released 0.15 with this change that day.

@nicosommi

This comment has been minimized.

Contributor

nicosommi commented Jun 4, 2018

@StreetStrider

That isn't nice in chaining API.

If you use transacting with an empty value "explicitly", then you should call all available methods explicitly as well according to that reasoning.

BTW chaining is not an impediment for this... as I said, you can omit the call.

const q = knex().select()
if (t) q.transacting(t)
q.where()...

"If an API leads you to bad usage, is a bad API."

Regarding to this being a breaking change or not, I think that it was already discussed enough on this issue.

@StreetStrider

This comment has been minimized.

StreetStrider commented Jun 4, 2018

@nicosommi, no, you don't get my opinion. I mean, the accepted solution to throw on empty value is OK for me, because it's explicit (not mine, which is implicit, of course). I will handle empty values on my side.

However, I think that implicit solution is better in this particular case, on chaining API, since you don't need to break the chain.
There's no much sense to have an chaining API, which main purpose is to not to pass intermediate value all over the place, and to write snippets like yours or mine, where q is stored. Besides, knex query builder is stateful, so having multiple refs to query might lead to user thinking it is immutable and can be reused (in fact, it can't, you need to create fresh instance every time or clone). So, it's not an impediment, but by breaking the chain you create more confusion.

@StreetStrider

This comment has been minimized.

StreetStrider commented Jun 4, 2018

forces us to add an if statement to previously linear code

Agreed, so we translate complexity from library code to user code, which isn't good.

@wubzz

This comment has been minimized.

Collaborator

wubzz commented Jun 4, 2018

Honestly I don't think this should have been changed to throw an error. Needless to say this adds inconsistencies against how other functions handles "invalid input". Usually we just helpers.warn() to notify the user, but we don't throw unless absolutely neccessary. In this case I wouldn't consider it a necessity.

@elhigu

This comment has been minimized.

Collaborator

elhigu commented Jun 4, 2018

@wubzz I have to disagree with that. We already changed long ago to undefined bindings to throw an error, so this change was comparable to that.

Some people were using that as a feature also, but I have no regrets on denying it, since it was pretty dangerous behaviour. This might not be that dangerous, but just adding warning would give a bit mixed signal saying that "we kind of support writing that kind of code" (which actually makes your app more prone to errors). There are some people who needs to modify their code to be like this: #2494 (comment) but to me that is a small price when this change help all that people who are passing undefineds to that method by accident.

IMO knex should throw a lot more errors when invalid parameters are passed to API methods.

@wubzz

This comment has been minimized.

Collaborator

wubzz commented Jun 4, 2018

@elhigu To me there's a clear difference: Undefined bindings is something we simply could not and cannot handle. Everything broke - how should the query be structured? This issue pales in comparison imho, since technically this didn't break anything.

I'm not saying it should be reverted, but I want to see consistency. For example .limit(null), .limit('test') does not throw, it warns. Why? There are many more examples.

For consistency we should change every single one of these functions to do one or the other, and not mix.

@elhigu

This comment has been minimized.

Collaborator

elhigu commented Jun 4, 2018

@wubzz unefined bindings were handled earlier by dropping those parts of where clauses off from query. So there was a way how to handle them and people was using that as a feature. Nothing broke, except when that was done by accident.

Now with transacting() there was a way how to handle undefined transaction, which was "don't use transaction after all, but just grab a new connection from pool". Nothing broke, except when that was done by accident.

I don't see much difference in those cases (except that dropping parts of where was even more dangerous). I know this all depends where one draws the line. I suppose .limit() with bad arguments is older code, when knex never threw errors. IMO it should throw an error as well, just like many other places.

For consistency we should change every single one of these functions to do one or the other, and not mix.

Yes. To a style where invalid arguments throw an error :)

@wubzz

This comment has been minimized.

Collaborator

wubzz commented Jun 4, 2018

To say that nothing broke with undefined bindings is pretty far-fetched, at least for Postgres seeing as it previously threw runtime errors... Here's one example #1417

In any case sounds like a pretty big Pull Request incoming. A quick search shows 38 cases where we warn instead of throw. I'll try get to this today.

@elhigu

This comment has been minimized.

Collaborator

elhigu commented Jun 4, 2018

To say that nothing broke with undefined bindings is pretty far-fetched, at least for Postgres seeing as it previously threw runtime errors... Here's one example #1417

Yup that would have been far fetched. I'm glad I didn't wrote "Nothing broke", but "Nothing broke, if it was done intentionally." Passing .where('id', undefined) was perfectly fine back in days even with postgresql. Can't remember specifics of #1417 what were the circumstances / root cause for that. Maybe some other changes done in certain way started causing that or something like that. Too bad there wasn't test case in that PR.

In any case sounds like a pretty big Pull Request incoming. A quick search shows 38 cases where we warn instead of throw. I'll try get to this today.

Sounds good, hopefully there isn't too much obstacles on that road.

I once started (maybe 1.5 years ago) actually writing bunch of tests for testing that in every place correct parameters are checked and ended up stashing that work to the distant future.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment