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

Escaping values in migrations #799

Closed
pigulla opened this issue Sep 1, 2020 · 11 comments
Closed

Escaping values in migrations #799

pigulla opened this issue Sep 1, 2020 · 11 comments
Labels
enhancement New feature or request

Comments

@pigulla
Copy link

pigulla commented Sep 1, 2020

Is your feature request related to a problem? Please describe.
When writing migrations, it is not clear how the queries can be constructed safely. While a migration obviously does not contain user input and escaping might not be strictly necessary, it is arguably good form to do so anyway.

Describe the solution you'd like
Provide a built-in solution to escape values. Ideally the knex instance could be used but it is my understanding that this is not feasible due to its lack of support for MongoDB.

Describe alternatives you've considered
Currently I am using pg-format which works well enough, but that means tying my migrations to a specific database, which is not ideal.

Additional context
I've read this issue which asks a related question.

@pigulla pigulla added the enhancement New feature or request label Sep 1, 2020
@B4nan
Copy link
Member

B4nan commented Sep 1, 2020

In Migration class you have access to the AbstractSqlDriver (via this.driver), where you can either use the MikroORM query builder, or get the configured knex instance via this.driver.getConnection().getKnex().

@pigulla
Copy link
Author

pigulla commented Sep 1, 2020

Thanks for the super quick reply.

It is my understanding that the Knex instance can not be used until the next RC (as mentioned here) - is that correct?

When playing around with this this.driver.getConnection threw TypeError: this.driver.getConnection is not a function during runtime, but that's most likely due to me not using the latest RC (my bad). Now I ran into the escaya issue, but I will just hold still until RC5 is out :)

Thanks a lot for your work, looking forward to using it!

@pigulla pigulla closed this as completed Sep 1, 2020
@B4nan
Copy link
Member

B4nan commented Sep 1, 2020

It is my understanding that the Knex instance can not be used until the next RC (as mentioned here) - is that correct?

That is only if you want to execute a query, not if you use it to build a query for this.addSql().

@pigulla
Copy link
Author

pigulla commented Sep 1, 2020

How do I do that? As far as I know Knex' toSQL() or (toSQL().toNative()) returns the statement and its (unescaped) bindings as separate values so it is not a query I can pass to addSql.

@B4nan
Copy link
Member

B4nan commented Sep 1, 2020

You could use toString(), and we could easily allow passing params to addSql() too I guess.

@pigulla
Copy link
Author

pigulla commented Sep 1, 2020

Maybe I just need some more coffee, but I don't think this is how Knex works:

knex
    .insert({
        id: 42,
        foo: 'bar',
    })
    .into('mytable')
    .toSQL()
    .toNative() // or without this, no difference
    .toString()

just returns [object Object]. Apparently there are only pretty ugly workarounds for this (admittedly, an old ticket - couldn't find anything more recent regarding this issue).

@B4nan
Copy link
Member

B4nan commented Sep 1, 2020

knex
    .insert({
        id: 42,
        foo: 'bar',
    })
    .into('mytable')
    .toString()

@B4nan
Copy link
Member

B4nan commented Sep 1, 2020

But we could easily allow to pass the knex qb instance to addSql() too. I feel like the toString() method is more of an approximation - I am using that in the query logging and I know about some differences when it comes to object handling (Dates in particular are printed with lower precision, without timezone, and in sqlite it prints the date string while in fact the query is using int/timestamps under the hood (that is how sqlite3 does things internally afaik).

So I would definitely not suggest to do this. Personally I would just write the SQL myself, it's easier and you have absolute control over everything.

@pigulla
Copy link
Author

pigulla commented Sep 1, 2020

I'm fine with that, if that's the "official" way to do this. Thanks for the feedback!

B4nan added a commit that referenced this issue Sep 2, 2020
This adds a `Migration.getKnex()` shortcut as well as allows
using knex in `Migration.addSql()` and `Migration.execute()`.

Related: #799
@B4nan
Copy link
Member

B4nan commented Sep 2, 2020

rc.5 will allow to use knex in both execute and addSql methods, there is also a new shortcut to get knex.

@pigulla
Copy link
Author

pigulla commented Sep 2, 2020

Fantastic!

Do you have an ETA for rc.5 by any chance?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

2 participants