feat(clusterbuster): Revamp queries to remove Slonik and allow overwrite of the base query #52
Conversation
…ite of the base query Now you can overwrite base query and you can have the data in multiple tables joined in the base query
|
🎉 This PR is included in version 2.8.0 🎉 The release is available on: Your semantic-release bot 📦🚀 |
There was a problem hiding this comment.
I like the flexibility this gives! Removing the dependency on an external lib is also an improvement.
@cosmin-petrescu small note, the reason for adding slonik was to prevent SQL injection (it handles value binding). Perhaps it would be good to test for that? I think x,y,z should be passed as value instead of directly in SQL, which is easy to do without slonik (see my other comments).
Additionally, maybe this new property could use a small doc mention?
| attributes, | ||
| }: ITileQuery) => ` | ||
| SELECT | ||
| ST_AsMVTGeom(ST_Transform(${geometry}, 3857), TileBBox(${z}, ${x}, ${y}, 3857), ${extent}, ${bufferSize}, false) AS geom, |
There was a problem hiding this comment.
perhaps z, x, y should be $1, $2, $3 and passed to the query as values instead of directly as SQL strings in the query
There was a problem hiding this comment.
That would be an idea or we can make sure x, y and z are numbers. This way we make sure is not SQL injection. I'll make a PR for that.
| }); | ||
| const result = await pool.query(query.sql, query.values); |
There was a problem hiding this comment.
even without slonik, values should be used instead of directly putting SQL strings in the query
Now you can overwrite base query and you can have the data in multiple tables joined in the base query