Can sql.unsafe be assumed if sql is used?

[markcellus]

I've noticed that in version 33 sql can no longer be used, or you'd get the error mentioned in #434.

While I understand the stronger type checking, in order to upgrade to 33 from 32, I now have to change sql to sql.unsafe throughout quite a few projects. It would be nice if large refactors like this could be avoided as evidenced in #437. Especially given that some projects are javascript-only and want to handle their own type enforcement (e.g. Types via JSDocs). Can unsafe be assumed if no sql method is used? I may be misunderstanding, but isn't that how it worked before? 🤔

[gajus]

Can unsafe be assumed if no sql method is used?

No, because the intent here is to communicate that what you are doing is unsafe.

However, nothing stopping you from just running mass replace of all sql to sql.unsafe.

[mmkal]

Conceptually this is backwards, IMO. After 6 years of existing as a perfectly-good SQL client, a feature was added and the existing, battle-tested usage was rebranded "unsafe".

sql.withZodValidation(...)`select * from foo` and sql`select * from foo` would have been more accurate, more informative, more honest about the coupling with an unrelated library, and non-breaking.

It also would have been easy to have an opinionated, company-specific wrapper-layer which aliased export const sql = slonik.sql.withZodValidation and export const sqlWithoutZod = slonik.sql in some kind of @contra/slonik internal library - without forcing all your users to go along with all of Contra's opinions which are appropriate for Contra's workflows. But I guess we're all using @contra/slonik without knowing it 🤷. A little alienating, but it's your library and your call @gajus!

[gajus]

Regarding the wrapper, the same could be said about the inverse – no one is stopping you from extending sql locally to allow previous behavior.

[gajus]

However, if we take a step back, the whole reason I started working on Slonik is because over and over again I saw poor SQL integrations in Node.js projects. I think it is a combination of several factors, one of which is that for the longest time NoSQL was the go-to backend for Node.js projects, i.e. general understanding SQL by JavaScript engineers has been lacking. This is why I saw the need to create an abstraction that protects engineers against SQL injections, unsafe transaction handling, and performance issues.

I dare say that Slonik has done a decent job with regards to all of the mentioned objectives. However, one issue remained: TypeScript types not accurately reflecting the result of queries. This has gotten me personally in problems repeatedly over the years to the point where I realized that Slonik is no longer fulfilling the earlier promise. This became especially clear in Contra where many engineers were modifying queries and schema in parallel. However, by no means this is an issue unique to Contra – it is an issue for any non-trivial project that utilizes Slonik.

With all of that said, I understand how a change like this can alienate existing users and contributors. And I am very conscious of how the success of Slonik depends on earning trust of its users and contributors. I stand by that this change is good for the long term adoption of Slonik and health of the community, however I also realize how there is a need to start talking about the API stability and governance. This is being discussed in this issue #450

[nfrolov]

I appreciate your response!

However, open to alternative suggestions.

I see keeping sql tag as it was before and making it return non-validated data with an {} and unknown types as best solution. In TypeScript unknown and {} will force developers to check and validate query results. For that sql.type or in any other available option could be used.

In my opinion Zod or any other concrete schema validation library should not be integrated into Slonik. Everyone should be able to use whatever validating library they prefer. Issues like that #449 would not exist.

Could Slonik just ask for schema validation function and infer all required types from it?

If suffix is required, then something like untyped would be more suitable.

Regardless, the intent here is very simple: Engineers reading the code should question if they are using the right method. unsafe forces reader to investigate "why we are using an unsafe method", and the answer is that it is missing validation.

To be strict, validation will not make query "safe". Especially if you forget to add validating interceptor. Without it Slonik would just type cast data, giving false feeling of valid data and type safety.

[antonversal]

Thanks, @gajus, for the great library. We have used it in production for a few years already. However, the change with Zod is frustrating us. I'm not talking about the need for runtime validation, and it's a great idea, but rather about the hard dependency on Zod. We do not want to use Zod; we already have a validation library and do not need one more. We now use the wrapper, hoping the zod dependency would be broken.

[bootrino]

I don't seem to be able to find the documentation for the change to sql.unsafe.

Would someone mind directing me please?

Also, is this change in all the other documentation for slonik?

[gajus]

What's the question?