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

fix: EXPOSED-662 SchemaUtils.listTables() returns empty list & closes db connection #2331

Merged
merged 5 commits into from
Jan 10, 2025

Conversation

joc-a
Copy link
Collaborator

@joc-a joc-a commented Dec 10, 2024

Detailed description:

  • Why:
    Changes made in PR fix: EXPOSED-625 SchemaUtils.listTables() retrieves tables for the default schema only #2301 were causing the database connection to close as a side effect and SchemaUtils.listTables() was returning an empty list (if the table name cache was not filled before invoking it).
    Table names are cached lazily by a custom map that cannot be iterated over. All calls to getAllTableNamesCache() (or its underlying metadata field tableNames) must be followed by getValue(key) to ensure a metadata query is sent by default to fill the cache if empty.
  • How:
    • Revert this commit to get original behavior of SchemaUtils.listTables()
    • Add a new method SchemaUtils.listTablesInAllSchemas() that accomplishes the new behavior
    • Add an associated method DatabaseDialect.allTablesNamesInAllSchemas(), since some users found this issue by using DatabaseDialect.allTablesNames() directly

Type of Change

Please mark the relevant options with an "X":

  • Bug fix

Affected databases:

  • All

Checklist

  • Unit tests are in place
  • The build is green (including the Detekt check)
  • All public methods affected by my PR has up to date API docs
  • Documentation for my change is up to date

Related Issues

EXPOSED-622

Reverted change - EXPOSED-625

joc-a and others added 2 commits January 9, 2025 06:51
For some unclear reason, the changes made [here](008faf3) were causing the database connection to close. This fixes that by adding a new function `tableNamesForAllSchemas()` and invoking that in `allTablesNames()`.
… db connection

- Revert change to VendorDialect.allTablesNames() so that both it and SchemaUtils.listTables()
 has original behavior (only returns lists in current schema)
- Add new SchemaUtils method to alternatively return list of all table names in all
schemas.
- Adjust tests to invoke listTables() without prior call to exists().
@bog-walk bog-walk changed the title fix: EXPOSED-662 SchemaUtils.listTables() closes connection to db fix: EXPOSED-662 SchemaUtils.listTables() returns empty list & closes db connection Jan 9, 2025
@bog-walk bog-walk marked this pull request as ready for review January 9, 2025 12:02
@bog-walk bog-walk requested review from e5l and obabichevjb January 9, 2025 12:06
bog-walk and others added 3 commits January 9, 2025 07:27
… db connection

- Fix KDocs for new SchemaUtils method
- Override CachableMapWithDefault properties to throw unsupported exception &
prevent future similar issues
… db connection

- Undo changes to CachableMapWithDefault
Copy link
Member

@e5l e5l left a comment

Choose a reason for hiding this comment

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

Please add mention of the breaking change in the release notes

@obabichevjb obabichevjb merged commit 9105cc1 into main Jan 10, 2025
5 checks passed
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.

4 participants