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

api : fix/optimizes utils.rs #45

Open
wants to merge 9 commits into
base: main
Choose a base branch
from

Conversation

1182658898
Copy link

Pull Request Summary

Summary

This PR fixes potential panics in pagination logic, enhances JSON conversion by handling nullable fields and logging unhandled PostgreSQL types, and optimizes performance by reducing redundant operations.

Changes

  1. Pagination Logic:

    • Ensured total_pages is at least 1 to prevent panics when total_count is 0.
  2. rows_to_json Enhancements:

    • Cached column types and names to avoid repeated access.
    • Handled NULL values appropriately in JSON output.
    • Added logging for unhandled PostgreSQL types.
  3. General Improvements:

    • Maintained existing functionality of get_column_names.
    • Improved code readability and maintainability with concise logic.

Benefits

  • Stability: Prevents runtime crashes by handling edge cases in pagination.
  • Data Integrity: Accurately represents NULL values and logs unsupported types for better debugging.
  • Performance: Reduces unnecessary computations by caching column information.
  • Maintainability: Easier to extend and debug with clear logging and streamlined code.

api/src/utils.rs Outdated Show resolved Hide resolved
@jhheider jhheider changed the title docs : fix/optimizes utils.rs api : fix/optimizes utils.rs Jan 2, 2025
jhheider
jhheider previously approved these changes Jan 3, 2025
@jhheider
Copy link
Member

jhheider commented Jan 3, 2025

@1182658898
Copy link
Author

After many attempts, I found that the tokio version is insufficient, so it cannot be used directly. If it is upgraded, there may be other effects. My suggestion here is to restore this part to the previous one or use my previous more repetitive method to judge the empty space. I have practiced it and there is no problem, but the code may look a bit repetitive.

@jhheider
Copy link
Member

jhheider commented Jan 4, 2025

tokio (v1) and tokio-postgres (v0.7) are both the newest version. but i have no problem upgrading dependencies as needed.

however, shouldn't it just work to get the types as Option<T> on L39-60?

                        Type::INT2 => json!(row.get::<_, i16>(i)),
...
                        Type::JSON | Type::JSONB => row.get::<_, Option<Value>>(i),
                        Type::UUID => json!(row.get::<_, Option<Uuid>>(i).map(ToString::to_string))

?

@1182658898
Copy link
Author

tokio-postgres

OK, this is also a good method, it works very well, I think is_null is more concise, so I have been thinking about this problem

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