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

Add checking for hh/where when (seq clauses) is nil #413

Closed
slipset opened this issue Jun 24, 2022 · 5 comments
Closed

Add checking for hh/where when (seq clauses) is nil #413

slipset opened this issue Jun 24, 2022 · 5 comments
Assignees

Comments

@slipset
Copy link

slipset commented Jun 24, 2022

(-> (hh/delete-from :foo)
      (hh/where xs))

Is a potential foot gun when (seq xs) is nil, since it translates to DELETE FROM foo
It would be nice to add detection for this in the :checking functionality described in https://cljdoc.org/d/com.github.seancorfield/honeysql/2.2.891/doc/getting-started?q=checking#format-options

I'd be happy to work on a PR for this
Having looked at the code, I might have been a bit optimistic with regards to my happiness to work on a PR for this.
The checking bit is done in the format part of honey, and the format bit is somewhat involved it seems.
It would be much easier it seems to do this checking in hh/where :)

@seancorfield
Copy link
Owner

You can't do the checking in the helpers -- not everyone uses them and there are many ways to build the data structure that is the DSL.

The checking needs to be added to the formatters for :delete-from and :update in a way that will catch when there are no qualifying conditions, but I will need to do quite a bit of digging/analysis of SQL syntax implications before I can do that.

As usual, with SQL stuff, it's harder than it looks 😄

@seancorfield
Copy link
Owner

Adding checks to :delete, :delete-from, and :update for a non-empty :where clause is relatively straightforward but I don't want :checking :basic or :checking :strict to invalid legal code so I'm asking on Slack about it.

@orestis
Copy link

orestis commented Aug 4, 2022

We are using :delete-from with no where clause, in a single place: a full-sync ETL pipeline that first clears all the rows, then re-inserts them from the source DB. I wouldn't mind having to add a :where [:= 1 1] or similar, assuming that has no performance implications in most DBs.

I think doing some checking is a good way to avoid messing up during REPL excursions.

@slipset
Copy link
Author

slipset commented Aug 4, 2022

@orestis for such a use case, truncate table foo might be a better solution.

@seancorfield
Copy link
Owner

This has been added as a :basic check.

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

No branches or pull requests

3 participants