Skip to content

Refactor: Fix unwrap issues, dynamo pagination loops, and enforce CLI validation - #41

Closed
subbareddypalagiri wants to merge 2 commits into
EpicGames:mainfrom
subbareddypalagiri:main
Closed

Refactor: Fix unwrap issues, dynamo pagination loops, and enforce CLI validation#41
subbareddypalagiri wants to merge 2 commits into
EpicGames:mainfrom
subbareddypalagiri:main

Conversation

@subbareddypalagiri

@subbareddypalagiri subbareddypalagiri commented Jun 20, 2026

Copy link
Copy Markdown
Contributor

Hey team! This Pull Request bundles a few structural backend improvements and cleanups:

1. Replace unwrap() with expect() in lore/build.rs

This is a small refactor, but it ensures that if the build script ever fails (for example, missing OUT_DIR environment variables), it will provide a clear, actionable error message rather than a generic panic.

2. Fix Degenerate DynamoDB Pagination Query Bug

Currently, combining Select::Count with a limit in paginated queries causes DynamoDB to fetch the total count one row at a time, resulting in a degenerate performance loop. This adds a strict guard clause to query_paginated to panic/fail-fast if this combination is used, protecting cloud resources.

3. Remove Legacy ENABLE_QUERY_PAGINATION Tech Debt

Pagination support has been stable for some time. This removes the legacy environment variable check (ENABLE_QUERY_PAGINATION) and its associated TODO(jcohen), hardcoding the pagination logic to always run and reducing technical debt.

4. Mutually Exclusive CLI Logging Arguments (Closes UCS-12558)

Resolves the open Jira ticket TODO(UCS-12558): Cleanup logging args. The CLI previously allowed contradictory logging arguments (e.g. running with --debug and --silent simultaneously). This adds conflicts_with_all bounds to the clap parser for level, debug, and silent to strictly enforce mutually exclusive input validation.

This is my first major contribution here, looking forward to any feedback!

@subbareddypalagiri subbareddypalagiri changed the title refactor: replace unwrap() with expect() in lore/build.rs Refactor: Fix unwrap issues, dynamo pagination loops, and enforce CLI validation Jun 20, 2026
Comment thread lore-aws/src/dynamodb.rs
T: DynamoDbQuery + 'static,
{
if matches!(query.select(), Some(Select::Count)) && query.limit().is_some() {
panic!("Combining Select::Count and a limit is disallowed for paginated queries to avoid fetching count one row at a time.");

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

A panic is not an appropriate way to handle this. This should either be a combination that's structurally not possible and verified at compile time or it should be a properly handled runtime error.

@mjansson

Copy link
Copy Markdown
Collaborator

I would prefer if these unrelated changes are all broken up into separate PRs (and don't forget the DCO sign-off, see the contributing doc for instructions)

@subbareddypalagiri

Copy link
Copy Markdown
Contributor Author

Thank you both @ragnarula and @mjansson for the thorough review and constructive feedback! You are completely right on all points.

I have gone ahead and addressed your concerns:

  1. Error Handling: I removed the panic!() in dynamodb.rs and replaced it with a proper Result::Err using aws_sdk_dynamodb::error::SdkError::construction_failure so that the server fails gracefully on pagination limit conflicts.
  2. Separation of Concerns: I completely tore down this massive PR and broke the 4 unrelated fixes into their own targeted, hyper-focused Pull Requests.
  3. DCO Sign-offs: I have ensured that all the new commits are properly signed off (Signed-off-by) to comply with the project's contributing guidelines.

To keep the history clean and to respect your workflow, I will be closing this tangled PR now in favor of the new, separated ones.

Thanks again for your time and for maintaining this awesome project!

Best regards,
Palagiri Subba Reddy

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

Labels

None yet

Development

Successfully merging this pull request may close these issues.

3 participants