Skip to content

Improve Go query error handling and add non-panicking Get APIs#99

Open
Uzer007-admin wants to merge 1 commit into
Restream:masterfrom
Uzer007-admin:query-error-handling-pr
Open

Improve Go query error handling and add non-panicking Get APIs#99
Uzer007-admin wants to merge 1 commit into
Restream:masterfrom
Uzer007-admin:query-error-handling-pr

Conversation

@Uzer007-admin

Copy link
Copy Markdown
Contributor

Summary

This PR makes error handling in the Go query builder more predictable. Errors raised while building a query are stored on the root Query and returned when the query is executed instead of causing an immediate panic.

The following error-returning APIs are added for fetching a single item:

  • GetErr
  • GetErrCtx
  • GetJsonErr
  • GetJsonErrCtx

The existing Get, GetCtx, GetJson, and GetJsonCtx methods preserve their panic-on-error behavior and are implemented as wrappers around the new methods.

Changes

  • Propagate value serialization failures through the query API.
  • Replace immediate panics for invalid query builder operations, including:
    • nil subqueries, expressions, and KNN parameters;
    • unsupported expressions;
    • invalid bracket sequences;
    • repeated execution and execution of closed queries;
    • invalid Join, Merge, and On calls;
    • unsupported values passed to Where, Sort, Set, or SetObject.
  • Make Exec, ExecToJson, Delete, and Update surface accumulated builder errors through their existing error-returning APIs.
  • Preserve the first error raised while building a query.
  • Make repeated query close operations safe and idempotent.
  • Return ErrWrongType instead of panicking when a value with an incompatible Go type is passed to write or transaction APIs.
  • Return an OpenNamespace error instead of panicking on invalid collate or expire_after namespace tags.
  • Handle Set(field, nil) without a reflection panic.
  • Remove query allocation/close tracing controlled by REINDEXER_GODEBUG.

API and behavior changes

This PR contains intentional API and behavior changes:

  • adds GetErr, GetErrCtx, GetJsonErr, and GetJsonErrCtx;
  • removes the exported IFunction interface and SerializeFunction helper;
  • FlatArrayLen and Now no longer implement IFunction, while remaining supported query expressions;
  • WhereExpressions now accepts only the built-in supported expression types. Custom IExpression implementations produce an unsupported-expression error;
  • several query misuse cases now return errors through iterator/error APIs instead of panicking;
  • the existing non-Err Get* methods continue to panic on errors.

Tests

Added coverage for:

  • the new GetErr* APIs;
  • query builder error propagation;
  • repeated query execution;
  • panic compatibility of the existing Get* wrappers;
  • incorrect item types in regular, transaction, and async transaction operations;
  • invalid collate and expire_after namespace tags.

Comment thread test/pack_item_test.go
require.Panics(t, func() {
_ = DB.Upsert(ns, packItemWrongNamedItem{ID: 4})
require.Equal(t, rx.ErrWrongType, DB.Upsert(ns, packItemWrongNamedItem{ID: 4}))
require.NotPanics(t, func() {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

NotPanics looks redundant. We've already checked, that DB.Upsert has returned the specific error, so there are no ways it could throw a panic at the same time

(the same comment about other tests nearby)

Comment thread expression.go
}

// FlatArrayLen is the flat_array_len(field) expression; implements IFunction and IExpression.
// FlatArrayLen is the flat_array_len(field) expression.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's the idea behind removing the comment?

Comment thread query.go
// Check IExpression interface and list of implementations nearby for more details.
func (q *Query) WhereExpressions(left IExpression, condition int, right IExpression) *Query {
q.ser.PutVarCUInt(queryExpressions)
left.Serialize(&q.ser)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I personally prefer the old way with interface: it looked more clean, than the current inlined implementation. Current version has too much duplicated code. Maybe you could return the Serialize call and supplement it with the setErr call?

Comment thread query.go
q = q.root
}
if q.closed {
q.panicTrace("Close call on already closed query")

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Probably this panic was actually usefull: it doesn't depend on the user's actions, it's more like debug check for RX developers. If you've faced this specific panic, it usually means, that something is wrong with the connector itself. Do not think, that we should silently ignore double close

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants