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

deprecate: Raise deprecation level of currentScheme property #1874

Merged
merged 3 commits into from
Oct 5, 2023

Conversation

bog-walk
Copy link
Member

@bog-walk bog-walk commented Oct 2, 2023

Raise deprecation level of ExposedDatabaseMetadata.currentScheme from WARNING to ERROR.

This property was initially deprecated after this PR introduced the possibility to change the schema in runtime.

The goal of this fix is to keep the property private in JdbcDatabaseMetadataImpl, which has 2 complications:

  1. VendorDialect functions use it to filter existing table names:

    • This is replaced by a new fun tableNamesByCurrentSchema(), which abstracts away the property from the core module. Ideallly this function should only return a List<String>, but it doesn't because of complication 2 (below).
  2. VendorDialect.tableExists() uses the property directly, as does 1 unit test:

    • So tableNamesByCurrentSchema() instead returns a Pair, which maybe defeats the purpose of making it private.
    • Alternatives:
      • Put the entire tableExists() into JdbcDatabaseMetadataImpl. This would require refactoring of the cache in VendorDialect and Table.exists().
      • JdbcDatabaseMetadataImpl.schemaNames could be refactored to always set the current schema name as the first list element, so it could be reliably found using the cache in VendorDialect. This seems prone to error if any future changes are made.

Raise deprecation level of ExposedDatabaseMetadata.currentScheme from ERROR to
WARNING.

This property was meant to be private/protected within classes that implement
the interface, but has been used in VendorDialect` and 1 unit test.

A new abstract fun tableNamesByCurrentSchema() is introduced to abstract away
the schema from the core module. Ideally this function would return a List<String>
but tableExists() needs to know about the current scheme name, so it instead
returns a Pair to provide that.
Change name of private property to no longer have underscore prefix.
@bog-walk bog-walk requested review from e5l and joc-a October 5, 2023 01:40
@e5l
Copy link
Member

e5l commented Oct 5, 2023

From WARNING to ERROR

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.

lgtm, please check comments

Add data class SchemaMetadata as nre return type of tableNamesBySchema().
@bog-walk bog-walk merged commit 0eb0ac1 into main Oct 5, 2023
5 checks passed
@bog-walk bog-walk deleted the bog-walk/deprecate-current-scheme branch October 5, 2023 21:44
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