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

ListEvaluationHistory query is generated dynamically. #3796

Closed
wants to merge 1 commit into from

Conversation

blkt
Copy link
Contributor

@blkt blkt commented Jul 5, 2024

Summary

This is an alternative implementation of the ListEvaluationHistory endpoint using goqu sql builder library. Generated queries are equivalent to the one implemented using sqlc.

Some of the code from internal/db/eval_history.go is taken from sqlc-generated internal/db/eval_history.sql.go in order to minimize code changes for this POC. Additionally, the generated query can be further optimized with this approach using data from filters, e.g. by reducing the number of joins when filtering by entity type.

The big downside is that the amount of glue code to write roughly doubles because of the need for custom structs and db access layer.

Change Type

Mark the type of change your PR introduces:

  • Bug fix (resolves an issue without affecting existing features)
  • Feature (adds new functionality without breaking changes)
  • Breaking change (may impact existing functionalities or require documentation updates)
  • Documentation (updates or additions to documentation)
  • Refactoring or test improvements (no bug fixes or new functionality)

Testing

Currently untested.

Review Checklist:

  • Reviewed my own code for quality and clarity.
  • Added comments to complex or tricky code sections.
  • Updated any affected documentation.
  • Included tests that validate the fix or feature.
  • Checked that related changes are merged.

@blkt blkt self-assigned this Jul 5, 2024
AlertDetails sql.NullString `json:"alert_details"`
}

func (q *Queries) ListEvaluationHistory(ctx context.Context, params ListEvaluationHistoryParams) ([]ListEvaluationHistoryRow, error) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is the bulk of the change.

This is an alternative implementation of the ListEvaluationHistory
endpoint using goqu sql builder library. Generated queries are
equivalent to the one implemented using sqlc.

Some of the code from internal/db/eval_history.go is taken from
sqlc-generated internal/db/eval_history.sql.go in order to minimize
code changes for this POC. Additionally, the generated query can be
further optimized with this approach using data from filters, e.g. by
reducing the number of joins when filtering by entity type.

The big downside is that the amount of glue code to write roughly
doubles because of the need for custom structs and db access layer.
@blkt blkt force-pushed the history-service-impl-goqu branch from af732df to e6f8be8 Compare July 5, 2024 15:28
@eleftherias
Copy link
Contributor

Thanks for writing this out @blkt! I find this code is easier to reason about because it's if statements in Go code, rather than being SQL. What's your takeaway from this? Do you have a preference between the goqu generated queries and the previous sqlc implementation?

@blkt
Copy link
Contributor Author

blkt commented Jul 8, 2024

Hey @eleftherias, thanks for reviewing this! 🙇

I prefer by far the sql-builder approach for several reasons, I don't think there's much to discuss from the technical perspective: generating code is almost always a better approach. Additionally, goqu has extensive support for Postgres, like WITH statements, ON CONFLICT clauses, and so on.

On the other hand, making this PR ready for merging requires more work to expose routines not generated by sqlc from the db.Querier interface.
Most importantly, once we start generating queries dynamically we loose the possibility of checking the correctness of those statement by inspection (i.e. review), and it becomes pretty much mandatory to write integration tests checking that the generated statements behave correctly. This entails test containers and potentially CI work.

@blkt
Copy link
Contributor Author

blkt commented Jul 10, 2024

The team agreed on closing this in favor of #3784

@blkt blkt closed this Jul 10, 2024
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