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

Numerous flaws in Clean implementation for Oracle DB #1550

Closed
voidunit opened this issue Mar 12, 2017 · 7 comments
Closed

Numerous flaws in Clean implementation for Oracle DB #1550

voidunit opened this issue Mar 12, 2017 · 7 comments

Comments

@voidunit
Copy link
Contributor

@voidunit voidunit commented Mar 12, 2017

Flaws in Clean phase:
  • Protection from cleaning SYSTEM schema should be extended to all Oracle-maintained schemas. The list of schemas can be found as select schema, other_schemas from dba_registry.
  • Flag defaultSchemaForUser is computed wrongly: it shouldn't ignore case when compares the schema name with the deploy user name, because schema names are always quoted in the Flyway code (which is correct).
  • Implementation of OracleDbSupport#doCurrentSchemaName method is wrong: the correct SQL should be SELECT SYS_CONTEXT('USERENV', 'CURRENT_SCHEMA') FROM DUAL. New methods should be added to DbSupport class and its inheritors: getCurrentUser and doGetCurrentUser, which by default can return jdbcTemplate.getMetaData().getUserName() (which is not precise) and should be overridden. For Oracle it's SELECT USER FROM DUAL. This new method must be used for computing defaultSchemaForUser flag named above instead of doCurrentSchemaName.
  • The warning message with object types that cannot be cleaned due to Oracle limitations should be revised. AQ and Scheduler objects (and many many others) can be dropped if the deploy user has corresponding privileges. The only problems I know are related to Spatial metadata, DB links and old style Jobs. These objects still can be dropped not by the owner with a temporary local procedure or some SYS-owned packages, but this should be done in a callback, I suppose.
  • Implementation of OracleDbSupport#doAllTables to fix #178 is fallible. It should be corrected. There's no use to count references. Instead this graph should be traversed in correct direction.
  • Using ALL_ data dictionary views. According to this article it is recommended to use DBA_ views when possible for better performance. Availability of DBA_ views can be checked before running Clean command. Superusers allowed to drop/create others' objects usually have access to DBA_ views.
  • The body of doClean() method should be refactored (and this remark is related to all the Schema implementations): there are a lot code duplication (repeated typical for-loops) and violation of SRP (clean logic for each and every object type/metadata is stored in the same class). This makes doClean() less supportable.
  • The list of object types to be dropped should be extended significantly. There are about 40 types that can be dropped (see DBA_OBJECTS view source for all possible object types). There are still some types that cannot be dropped: some objects are always SYS-owned, some can be dropped only by their owner, some probably shouldn't be dropped at all - like DB links and Credentials - this is to be discussed.
Flaws not directly related to Clean phase but noticed in the mentioned classes:
  • Using count(*) for checking whether a query returns rows is not efficient. This should be replaced with exists(...) SQL function or a new method in JdbcTemplate class, which would only check and return .next() on the returned ResultSet of a query

I can fix all these issues.

@voidunit
Copy link
Contributor Author

@voidunit voidunit commented Mar 12, 2017

#1306 can be fixed as part of this issue

@axelfontaine
Copy link
Contributor

@axelfontaine axelfontaine commented Mar 12, 2017

Thanks for pointing this out. One pull request per flaw would be awesome as it would make them easy to analyse and therefore quicker to get accepted.

@gpolet
Copy link

@gpolet gpolet commented Jun 2, 2017

The method that evaluates if flashback is available or not is not correct. It checks that DBA_FLASHBACK_ARCHIVE_TABLES exists in all_objects but it does not verify that the user has access to it. In case the user cannot select from that table, flyway.clean will fail with "ORA--00942: table or view does not exist".

@voidunit
Copy link
Contributor Author

@voidunit voidunit commented Jun 2, 2017

@gpolet It is granted to PUBLIC. Have you encountered this issue? I will add a check for accessibility anyway.

@gpolet
Copy link

@gpolet gpolet commented Jun 2, 2017

@vosolovskiy Hi, thanks for the quick feedback. I indeed faced this problem in Oracle. Not sure exactly why this differs from your situation in my Oracle installation. This might have been imposed by our DBA's.

@voidunit
Copy link
Contributor Author

@voidunit voidunit commented Jun 4, 2017

@gpolet Added a fix to this issue, see PR #1601

axelfontaine added a commit to flyway/flywaydb.org that referenced this issue Nov 27, 2017
@axelfontaine
Copy link
Contributor

@axelfontaine axelfontaine commented Nov 27, 2017

@vosolovskiy Thanks again for the fantastic contribution. Truly appreciated!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
3 participants
You can’t perform that action at this time.