Add fluent CQL builder #30
Conversation
jakub-id
requested review from
adamdickmeiss and
Copilot
and removed request for
adamdickmeiss
There was a problem hiding this comment.
Pull request overview
This PR adds a fluent builder API for programmatically constructing CQL queries with validation and automatic escaping of special characters to prevent injection attacks.
Changes:
- Introduces a new
cqlbuilderpackage with a fluent API for building CQL queries - Adds comprehensive test coverage including validation and security tests
- Updates README with usage examples and documentation
Reviewed changes
Copilot reviewed 3 out of 4 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| cqlbuilder/builder.go | Implements the fluent builder API with QueryBuilder, SearchBuilder, JoinBuilder, and ExprBuilder types supporting query construction, validation, and escaping |
| cqlbuilder/builder_test.go | Comprehensive test suite covering basic queries, boolean operations, prefixes, sorting, validation, query extension, and injection prevention |
| README.md | Adds new "Building CQL programmatically" section with usage example for the cqlbuilder package |
| .gitignore | Adds .DS_Store to ignore macOS system files |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| import ( | ||
| "fmt" | ||
|
|
||
| "github.com/indexdata/cql-go/cql" | ||
| "github.com/indexdata/cql-go/cqlbuilder" | ||
| ) |
There was a problem hiding this comment.
The import statement is missing the "os" package, which is required for the os.Stderr reference in the example code at line 55. This will cause a compilation error if someone tries to use this example.
adamdickmeiss
left a comment
adamdickmeiss
left a comment
There was a problem hiding this comment.
Way too low coverage < 50 %. I would expect 100% which should be possible with no external code is involved eg pg
| // NewQuery creates a new query builder. | ||
| func NewQuery() *QueryBuilder { | ||
| return &QueryBuilder{} | ||
| } |
There was a problem hiding this comment.
Why not New or NewBuilder ?
| // Prox starts a PROX boolean expression. | ||
| func (eb *ExprBuilder) Prox() *JoinBuilder { | ||
| return eb.join(cql.PROX) | ||
| } |
There was a problem hiding this comment.
The operators are infix.. But without a grouping operator I don't see how you represent queries such as (a or b) and (d or b). If they were prefix or postfix we would not need grouping op.
| if sb.err != nil { | ||
| return &ExprBuilder{finish: sb.finish, build: sb.build, qb: sb.qb, err: sb.err} | ||
| } | ||
| if strings.TrimSpace(term) == "" { |
There was a problem hiding this comment.
Why are empty terms not allowed ? Empty terms have specific meaning for pgcql as well as RMB.
cqlbuilder/builder.go
Outdated
| Index: sb.index, | ||
| Relation: sb.rel, | ||
| Modifiers: sb.mods, | ||
| Term: escapeValue(term), |
There was a problem hiding this comment.
I question why escapeValue used here (always). Is it the purpose of term that it must be able to represent any CQL query term? In that case, this will hinder that.. Eg searching for verbatim * which would be \* in CQL would be converted to \\* search for backslash and wildcard. I think there should be no conversion by default. We could have a RawTerm which would properly escape all the known masking characters recognized by CQL.
| return s | ||
| } | ||
| s = strings.ReplaceAll(s, "\\", "\\\\") | ||
| s = strings.ReplaceAll(s, "\"", "\\\"") |
There was a problem hiding this comment.
A bare " should be an error. It's that simple; except when used in RawTerm.
| Search("dc.title"). | ||
| Rel(cql.EQ). | ||
| Term("hello"). | ||
| Build() |
There was a problem hiding this comment.
Why not just assert.NoError and assert.Equal ?
|
|
||
| func TestBuilderValidation(t *testing.T) { | ||
| if _, err := NewQuery().Search("a").Term("").Build(); err == nil { | ||
| t.Fatalf("expected error for empty term") |
There was a problem hiding this comment.
Does not test that this is the error actually returned
| } | ||
|
|
||
| if _, err := NewQuery().Search("a").Rel("bogus").Term("x").Build(); err == nil { | ||
| t.Fatalf("expected error for invalid relation") |
There was a problem hiding this comment.
Same here. We don't know what error we got here.
adamdickmeiss
left a comment
adamdickmeiss
left a comment
There was a problem hiding this comment.
Fine with the clauses
No description provided.