RHIDP-14306: add saved prompts data access layer#2166
Conversation
Signed-off-by: Jordan Dubrick <jdubrick@redhat.com>
Signed-off-by: Jordan Dubrick <jdubrick@redhat.com>
Signed-off-by: Jordan Dubrick <jdubrick@redhat.com>
Signed-off-by: Jordan Dubrick <jdubrick@redhat.com>
Signed-off-by: Jordan Dubrick <jdubrick@redhat.com>
Signed-off-by: Jordan Dubrick <jdubrick@redhat.com>
Signed-off-by: Jordan Dubrick <jdubrick@redhat.com>
|
Warning Review limit reached
Next review available in: 41 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (2)
WalkthroughThe saved-prompt module now defines shared domain errors, validates quota, names, and content, and provides SQLAlchemy-backed create, list, and ownership-checked delete operations. Unit tests now cover persistence, conflicts, ordering, quotas, timestamps, and access control. ChangesSaved prompt domain
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
Suggested reviewers: Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (1 error)
✅ Passed checks (6 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
✨ Simplify code
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/utils/saved_prompts.py`:
- Around line 177-193: Update list_saved_prompts_by_user to prevent unbounded
result materialization by adding cursor-based pagination or an enforced hard
limit directly to this query. Preserve filtering by user_id and descending
created_at ordering, and ensure the protection is independent of write-path
quota enforcement.
- Around line 147-159: Update the saved-prompt creation flow around
validate_saved_prompt_quota and session.commit to make quota enforcement atomic
under concurrent requests. Serialize quota checks and inserts per user, or
enforce the limit with a database counter/serializable transaction and retry
conflicts, ensuring concurrent creates cannot exceed max_prompts_per_user.
- Around line 128-156: Update the public create path around the SavedPrompt
construction to validate and normalize name and content before persistence,
rather than storing raw caller values. Reuse the established saved-prompt
validators, ensuring blank and oversized values are rejected and names such as
"name" and " name " normalize identically before quota checking and insertion.
- Around line 158-165: Update the IntegrityError handling around session.commit
in the saved-prompt creation flow to translate only violations of the
uq_saved_prompt_user_name constraint into SavedPromptConflictError. Inspect the
exception for that constraint, preserving the existing conflict log and error
mapping when present, and re-raise any other IntegrityError unchanged.
In `@tests/unit/utils/test_saved_prompts.py`:
- Around line 62-63: Add a Google-style docstring to the nested _get_session
function documenting that it creates and returns a Session, including a Returns:
section with the Session type. Keep the existing session_factory() behavior
unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 86ed8d71-d62c-4b1d-a6b8-69cba8b3087a
📒 Files selected for processing (2)
src/utils/saved_prompts.pytests/unit/utils/test_saved_prompts.py
📜 Review details
⏰ Context from checks skipped due to timeout. (15)
- GitHub Check: black
- GitHub Check: unit_tests (3.13)
- GitHub Check: unit_tests (3.12)
- GitHub Check: bandit
- GitHub Check: integration_tests (3.12)
- GitHub Check: integration_tests (3.13)
- GitHub Check: build-pr
- GitHub Check: Pylinter
- GitHub Check: E2E: server mode / ci / group 1
- GitHub Check: E2E: library mode / ci / group 1
- GitHub Check: E2E: library mode / ci / group 3
- GitHub Check: E2E: server mode / ci / group 3
- GitHub Check: E2E: library mode / ci / group 2
- GitHub Check: E2E: server mode / ci / group 2
- GitHub Check: E2E Tests for Lightspeed Evaluation job
🧰 Additional context used
📓 Path-based instructions (6)
**/*
📄 CodeRabbit inference engine (Custom checks)
**/*: Flag meaningful O(n^2)+ algorithms on non-trivial inputs, including handlers and Kubernetes list operations.
Flag N+1 patterns that list items and then query once per item, including Kubernetes API and database access.
Flag expensive work inside loops, including API calls, JSON parsing, and regex compilation.
Flag unbounded growth in caches, watchers, or buffers when eviction or limits are missing.
Flag missing pagination or limits on list operations and API endpoints.
Flag secrets or tokens logged in plaintext or hardcoded in source.
Flag API endpoints missing authentication or authorization.
Flag injection vulnerabilities, including SQL injection, command injection, and path traversal.
Flag sensitive data leaked in API responses, WebSocket messages, or logs.
Flag Kubernetes Secrets and Red Hat secrets missing OwnerReferences.
Files:
tests/unit/utils/test_saved_prompts.pysrc/utils/saved_prompts.py
**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
**/*.py: Use absolute imports for internal Python modules.
Start every module with a descriptive docstring and uselogger = get_logger(__name__)for module logging.
Define shared constants in the centralconstants.pymodule, use descriptive comments, and annotate constants withFinal[type].
Use complete type annotations for function parameters and return values, modern union syntax, andtyping_extensions.Selffor model validators.
Document all modules, classes, and functions using Google-style Python docstrings, including applicable Parameters, Returns, Raises, and Attributes sections.
Use descriptive snake_case, action-oriented function names such asget_,validate_, andcheck_.
Avoid modifying input parameters in place; return a newly created data structure instead.
Useasync deffor I/O operations and external API calls.
Use standard logger levels appropriately: debug for diagnostics, info for normal execution, warning for unexpected conditions, and error for serious failures.
Name classes with PascalCase and use descriptive suffixes such asConfiguration,Error/Exception,Resolver, andInterfacewhere applicable.
Use ABC and@abstractmethodfor abstract interfaces; provide complete, specific type annotations for class attributes and avoidAny.
Files:
tests/unit/utils/test_saved_prompts.pysrc/utils/saved_prompts.py
tests/unit/**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
Use pytest for unit tests, shared fixtures in
conftest.py,pytest-mockfor mocks, andpytest.mark.asynciofor asynchronous tests; do not use unittest.
Files:
tests/unit/utils/test_saved_prompts.py
tests/**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
Maintain at least 60% unit-test coverage and 10% integration-test coverage.
Files:
tests/unit/utils/test_saved_prompts.py
**/*.{py,yaml,yml,json,toml}
📄 CodeRabbit inference engine (AGENTS.md)
Never commit secrets or keys; use environment variables for sensitive data.
Files:
tests/unit/utils/test_saved_prompts.pysrc/utils/saved_prompts.py
src/**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
Configuration models must extend
ConfigurationBase, whoseextra="forbid"behavior rejects unknown fields; use types such asOptional[FilePath],PositiveInt, andSecretStrwhere appropriate.
Files:
src/utils/saved_prompts.py
🧠 Learnings (2)
📚 Learning: 2026-06-24T13:45:37.249Z
Learnt from: Jdubrick
Repo: lightspeed-core/lightspeed-stack PR: 1971
File: src/utils/markdown_repair.py:31-36
Timestamp: 2026-06-24T13:45:37.249Z
Learning: In the lightspeed-stack repository, docstrings must use the section header name "Parameters:" (not "Args:") for function arguments, even if the project references Google Python docstring conventions. Ensure docstrings follow the project’s established "Parameters:" header format for any documented function parameters.
Applied to files:
tests/unit/utils/test_saved_prompts.pysrc/utils/saved_prompts.py
📚 Learning: 2026-07-06T15:26:18.398Z
Learnt from: Jdubrick
Repo: lightspeed-core/lightspeed-stack PR: 2071
File: src/models/config.py:2416-2422
Timestamp: 2026-07-06T15:26:18.398Z
Learning: In this repo’s Python code under src/**, don’t treat differences in string concatenation style as a style inconsistency when Black has effectively forced (or made clearer) use of explicit `+` string concatenation in multi-line logger/string expressions. If adjacent-literal implicit concatenation is avoided/changed specifically to accommodate Black’s formatting in these call sites, accept the `+` usage and don’t recommend converting it solely for consistency with nearby blocks that use implicit concatenation.
Applied to files:
src/utils/saved_prompts.py
🔇 Additional comments (3)
src/utils/saved_prompts.py (2)
1-58: LGTM!Also applies to: 61-117
120-174: 🩺 Stability & AvailabilityKeep this DAL synchronous.
src/utils/saved_prompts.pyis built onget_session() -> Session; there’s noAsyncSessionpath for this code, so the async-session refactor doesn’t apply here.> Likely an incorrect or invalid review comment.tests/unit/utils/test_saved_prompts.py (1)
1-61: LGTM!Also applies to: 65-67, 236-376
| try: | ||
| session.commit() | ||
| except IntegrityError as exc: | ||
| logger.debug( | ||
| "Saved prompt create conflict for user_id=%s", | ||
| user_id, | ||
| ) | ||
| raise SavedPromptConflictError("Saved prompt name already exists") from exc |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
ast-grep outline src/models/database/saved_prompts.py --items all
rg -nC4 'UniqueConstraint|unique=|ForeignKey|saved.prompt' \
src/models/database/saved_prompts.py
fd -a -t f -i 'saved.*prompt' . \
-x rg -nC3 'unique|constraint|user_id|name' {}Repository: lightspeed-core/lightspeed-stack
Length of output: 25310
Handle only the saved-prompt uniqueness violation here. This maps every IntegrityError to SavedPromptConflictError, so primary-key or other database failures are reported as duplicate names. Check for uq_saved_prompt_user_name and re-raise any other IntegrityError.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/utils/saved_prompts.py` around lines 158 - 165, Update the IntegrityError
handling around session.commit in the saved-prompt creation flow to translate
only violations of the uq_saved_prompt_user_name constraint into
SavedPromptConflictError. Inspect the exception for that constraint, preserving
the existing conflict log and error mapping when present, and re-raise any other
IntegrityError unchanged.
There was a problem hiding this comment.
Using UUID4 for the key here makes it extremely unlikely that we have a collision (if not almost impossible) on the PK field. The only other error that could be raised in that case is the duplicate name.
Signed-off-by: Jordan Dubrick <jdubrick@redhat.com>
1 participant
Description
Note: Endpoints will be added in future PRs
Type of change
Tools used to create PR
Identify any AI code assistants used in this PR (for transparency and review context)
Related Tickets & Documents
Checklist before requesting a review
Testing
Summary by CodeRabbit
New Features
Bug Fixes