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 utility method to check transactional table metadata #950

Conversation

Torch3333
Copy link
Contributor

@Torch3333 Torch3333 commented Jul 18, 2023

Context
ConsensusCommitUtils.isTransactionTableMetadata(metadata) is a utility method that returns true if the given table metadata is transactional.

Changes
This method is not working correctly when it verifies that the after column is present for a given before column. This PR fixes this wrong behavior.

@Torch3333 Torch3333 force-pushed the fix_util_method_to_test_if_it_is_transaction_table_metadata branch from 5ff3842 to 159fc45 Compare July 18, 2023 07:11
Comment on lines +120 to +123
&& !(nonPrimaryKeyColumn.startsWith(Attribute.BEFORE_PREFIX)
&& tableMetadata
.getColumnNames()
.contains(nonPrimaryKeyColumn.substring(Attribute.BEFORE_PREFIX.length())))) {
Copy link
Contributor Author

@Torch3333 Torch3333 Jul 18, 2023

Choose a reason for hiding this comment

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

nonPrimaryKeyColumn.startsWith(Attribute.BEFORE_PREFIX)
              && tableMetadata
                  .getColumnNames()
                  .contains(nonPrimaryKeyColumn.substring(Attribute.BEFORE_PREFIX.length())

were meant to be written inside parentheses since it first verifies that nonPrimaryKeyColumn starts with Attribute.BEFORE_PREFIX before it creates the substring nonPrimaryKeyColumn.substring(Attribute.BEFORE_PREFIX.length())

To gives more context, if nonPrimaryKeyColumn is not an after image column (verified by line 119), then the clause (line 120 to 123) verifies that nonPrimaryKeyColumn must be a before image column to be a transactional table metadata

@Torch3333 Torch3333 self-assigned this Jul 18, 2023
@Torch3333 Torch3333 changed the title Fix potential IndexOutOfBoundsException in ConsensusCommitUtils.isTransactionTableMetadata Fix utility method to check transactional table metadata Jul 18, 2023
@Torch3333 Torch3333 marked this pull request as ready for review July 18, 2023 08:19
Copy link
Collaborator

@brfrn169 brfrn169 left a comment

Choose a reason for hiding this comment

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

Good catch! LGTM! Thank you!

Copy link
Contributor

@komamitsu komamitsu left a comment

Choose a reason for hiding this comment

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

LGTM! Thank you!

Copy link
Contributor

@feeblefakie feeblefakie left a comment

Choose a reason for hiding this comment

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

LGTM! Thank you!

@feeblefakie feeblefakie merged commit e5bf6eb into master Jul 19, 2023
13 checks passed
@feeblefakie feeblefakie deleted the fix_util_method_to_test_if_it_is_transaction_table_metadata branch July 19, 2023 06:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants