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

[#3033] fix: sqlite3 drop/renameColumn() breaks with postProcessResponse #3040

Merged
merged 6 commits into from Mar 13, 2019

Conversation

Projects
None yet
4 participants
@elunic
Copy link
Contributor

elunic commented Feb 7, 2019

This fixes #3033, as described there.

@elhigu

This comment has been minimized.

Copy link
Collaborator

elhigu commented Feb 7, 2019

Test is missing and someone should check if that processResponse has been designed to be async method.

@whefter whefter force-pushed the elunic:fix/3033-sqlite3-dropcolumn branch from abfe836 to 52c4f68 Feb 7, 2019

@elunic

This comment has been minimized.

Copy link
Contributor Author

elunic commented Feb 8, 2019

@elhigu I added test cases for all the added functionality. That required adding a util, so tell me what you think of it all. Looking forward to it.

/wh

@whefter whefter force-pushed the elunic:fix/3033-sqlite3-dropcolumn branch from 8877520 to 85f998e Feb 8, 2019

[#3033] fix: sqlite3 drop/renameColumn() breaks with postProcessResponse
* When postProcessResponse is configured, and client.processResponse()
returns a Promise (e.g. for custom cases such as sqlite3 dropColumn()),
then that Promise is not awaited, but handed to postProcessResponse,
which might break is (e.g. with Objection's knexSnakeCaseMappers()).

* when reinserting data in the modified table, the rows are now being
handled with the "mapped" identifiers (instead of the unmapped)

* add tests, fix hasColumn

* add hasColumn tests for add mysql + snakeCaseMappers

@whefter whefter force-pushed the elunic:fix/3033-sqlite3-dropcolumn branch from 350ff63 to 7cfc3a3 Feb 11, 2019

@@ -0,0 +1,34 @@
import { isArray, isObject, snakeCase, camelCase, mapKeys } from 'lodash';

This comment has been minimized.

Copy link
@elhigu

elhigu Mar 3, 2019

Collaborator

I don't think this should be included here. You can use some dummy wrap / postprocess functions in tests which converts identifiers lower / uppercase to demonstrate the problem.

This comment has been minimized.

Copy link
@kibertoad

kibertoad Mar 3, 2019

Collaborator

can those be simply moved to tests then?

This comment has been minimized.

Copy link
@elhigu

elhigu Mar 3, 2019

Collaborator

There is no reason, why they should be in at all. Just using dummier implementations that does uppercase / lower case operations for identifiers and results should do the same effect and are simpler. This fix actually is not related specifically to mapping between snake case / camel case, but generally related to any mapping methods.

@elhigu
Copy link
Collaborator

elhigu left a comment

I think this is otherwise good, but I would like those snake case mappers to be removed. I don't want any mappers to be maintained in knex repo (even that they would be right now just used in tests someone might think later on that they are there for public use). Also real mapper functions are not needed for tests, dummy mappers can be used.

@kibertoad kibertoad requested a review from elhigu Mar 9, 2019

@kibertoad

This comment has been minimized.

Copy link
Collaborator

kibertoad commented Mar 9, 2019

@elhigu I addressed your review comments and also fixed table name processing which wasn't working correctly either; please give it another look!

this.tableName
}"`
);
this.trx.disableProcessing();

This comment has been minimized.

Copy link
@kibertoad

kibertoad Mar 9, 2019

Collaborator

this is an internal query which breaks if we e. g. uppercase fields name and sql.

kibertoad added some commits Mar 9, 2019

@kibertoad kibertoad merged commit 19926d8 into tgriesser:master Mar 13, 2019

1 of 2 checks passed

coverage/coveralls Coverage decreased (-0.06%) to 85.221%
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@elhigu

This comment has been minimized.

Copy link
Collaborator

elhigu commented Mar 29, 2019

Thanks! Looks good I suppose :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.