refactor: statement classifier

No labels

No milestone

No project

No assignees

1 participant

Notifications

Due date

The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference

noe.trevino/dadbod-ui.nvim#101

noe.trevino commented

2026-07-15 17:59:26 -05:00

Owner

What we need

One module that answers what kind of statement we are looking at, so the three features that need to know can stop each having their own regex pile.

Three questions, from the same place:

is it changing (does it mutate anything)
is it dangerous (DROP, or UPDATE / DELETE with no WHERE)
is it paginating (single plain select, no paging clause on it already)

Dangerous is a subset of changing. Everything dangerous mutates, not everything that mutates is dangerous.

Why

We already ask a version of question 3. paginator.lua has a guard that checks the statement is a single plain select and bails if it sees limit / offset / fetch / top / into / update / procedure, all so it can safely append a paging clause.

Question 1 and 2 are about to get asked by read only connections (#92) and dangerous statement detection (#96). Question 3 is about to get asked again by query transformers (#97), which needs the same "can i safely rewrite this" answer for all seven of its rewrites.

Thats four consumers of one question. Better as a module than as four copies that drift, we should probably just refactor now while we havent implemented anythig so we dont have to spend time going back and forth

Notes

Patterns live on the adapter spec, since dialects differ. Oracle has PURGE, clickhouse does ALTER TABLE ... DELETE, and mongo isnt sql at all so it cant be parsed by any of this and needs to say so rather than guess.

Worth splitting question 3 into two facts rather than one boolean. paginator.lua wants "plain select AND no paging clause already", but #97 needs those apart. Fetch all rows has to find and strip the paging clause, and row count wants to wrap SELECT ... LIMIT 10 in a subquery, which is fine even though you couldnt append a LIMIT to it. If it ships as one boolean #97 ends up re-parsing.

This is a refactor. No user visible change should come out of it on its own.

How

TBD, but i want a method on the module that is something like M.classify. This will be the main public method. it will return a struct like this:

---@class DadbodUI.Classification
---@field is_changing boolean
---@field is_dangerous boolean
---@field is_plain_select boolean
---@field is_paginating boolean
{
  is_changing = false,
  is_dangerous = false,
  is_plain_select = false, 
  is_paginated = false
}

This will let us go:

M.classify(query)

Query shouldnt be a string it should at least carry these fields:

adapter type
sql string

adapter type should just be the static enum of what is currently dadbod-ui.adapters.*

We should just turn this into an enum

Acceptance Criteria

one module, answers all three questions
patterns come from the adapter spec
is the statement changing
is the statement dangerous
is the statement a single plain select
does the statement already have a paging clause
paginator.lua uses it instead of its own guard
pagination behaviour doesnt change, existing tests still pass
adapters that arent sql say so instead of guessing
tested per adapter
adapters refactored into an enum (at least the built in ones)
implemented in all adapters where their cli allows
- postgres
- MySQL
- MariaDB
- SQL Server
- SQLite

Blocks

### What we need One module that answers what kind of statement we are looking at, so the three features that need to know can stop each having their own regex pile. Three questions, from the same place: 1. is it changing (does it mutate anything) 2. is it dangerous (DROP, or UPDATE / DELETE with no WHERE) 3. is it paginating (single plain select, no paging clause on it already) Dangerous is a subset of changing. Everything dangerous mutates, not everything that mutates is dangerous. ### Why We already ask a version of question 3. `paginator.lua` has a guard that checks the statement is a single plain select and bails if it sees limit / offset / fetch / top / into / update / procedure, all so it can safely append a paging clause. Question 1 and 2 are about to get asked by read only connections (#92) and dangerous statement detection (#96). Question 3 is about to get asked again by query transformers (#97), which needs the same "can i safely rewrite this" answer for all seven of its rewrites. Thats four consumers of one question. Better as a module than as four copies that drift, we should probably just refactor now while we havent implemented anythig so we dont have to spend time going back and forth ### Notes Patterns live on the adapter spec, since dialects differ. Oracle has PURGE, clickhouse does `ALTER TABLE ... DELETE`, and mongo isnt sql at all so it cant be parsed by any of this and needs to say so rather than guess. Worth splitting question 3 into two facts rather than one boolean. `paginator.lua` wants "plain select AND no paging clause already", but #97 needs those apart. Fetch all rows has to find and strip the paging clause, and row count wants to wrap `SELECT ... LIMIT 10` in a subquery, which is fine even though you couldnt append a LIMIT to it. If it ships as one boolean #97 ends up re-parsing. This is a refactor. No user visible change should come out of it on its own. ### How TBD, but i want a method on the module that is something like `M.classify`. This will be the main public method. it will return a struct like this: ``` lua ---@class DadbodUI.Classification ---@field is_changing boolean ---@field is_dangerous boolean ---@field is_plain_select boolean ---@field is_paginating boolean { is_changing = false, is_dangerous = false, is_plain_select = false, is_paginated = false } ``` This will let us go: ``` lua M.classify(query) ``` Query shouldnt be a string it should at least carry these fields: - adapter type - sql string adapter type should just be the static enum of what is currently `dadbod-ui.adapters.*` We should just turn this into an enum ### Acceptance Criteria - [ ] one module, answers all three questions - [ ] patterns come from the adapter spec - [ ] is the statement changing - [ ] is the statement dangerous - [ ] is the statement a single plain select - [ ] does the statement already have a paging clause - [ ] `paginator.lua` uses it instead of its own guard - [ ] pagination behaviour doesnt change, existing tests still pass - [ ] adapters that arent sql say so instead of guessing - [ ] tested per adapter - [ ] adapters refactored into an enum (at least the built in ones) - [ ] implemented in all adapters where their cli allows - [ ] postgres - [ ] MySQL - [ ] MariaDB - [ ] SQL Server - [ ] SQLite ### Blocks - #92 - #96 - #97

Rows
Columns

refactor: statement classifier #101

What we need

Why

Notes

How

Acceptance Criteria

Blocks