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

Jump to bottom

If table prefix is not specified, the database abilities diagnostic can fail #17771

Merged
merged 2 commits into from Jul 15, 2021
Merged

If table prefix is not specified, the database abilities diagnostic can fail #17771

merged 2 commits into from Jul 15, 2021

Conversation

diosmosis
Copy link
Member

Description:

If not table prefix is specified, the database abilities diagnostic can fail when it tries to batch insert into the option table. Fixed w/ backticks.

Fixes #17762

Review

  • Functional review done
  • Potential edge cases thought about (behavior of the code with strange input, with strange internal state or possible interactions with other Matomo subsystems)
  • Usability review done (is anything maybe unclear or think about anything that would cause people to reach out to support)
  • Security review done see checklist
  • Code review done
  • Tests were added if useful/possible
  • Reviewed for breaking changes
  • Developer changelog updated if needed
  • Documentation added if needed
  • Existing documentation updated if needed

@diosmosis diosmosis added the Needs Review PRs that need a code review label Jul 14, 2021
@diosmosis diosmosis added this to the 4.4.0 milestone Jul 14, 2021
@diosmosis diosmosis mentioned this pull request Jul 14, 2021
Closed
justinvelluppillai
justinvelluppillai reviewed Jul 15, 2021
View reviewed changes
Copy link
Contributor

@justinvelluppillai justinvelluppillai left a comment

Choose a reason for hiding this comment

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

I'm just getting started reviewing so I'll start participating by leaving comments without approvals so that someone else can also review.

It looks like this bug is because 'option' is a reserved word so the query needs the backticks to identify option as a table/column name. I guess that means tableInsertBatchIterate function will now work for table names with eg spaces or other reserved words but you have disallowed that with your preg_replace, which means spaces etc wouldn't work, which they already wouldn't have prior to this change.

Seems like a good fix to me.

@sgiehl sgiehl merged commit 5b9076d into 4.x-dev Jul 15, 2021
@sgiehl sgiehl deleted the batch-insert-option branch July 15, 2021 08:16
@mattab mattab changed the title use backticks since we can potentially insert batch into the option table If no table prefix is not specified, the database abilities diagnostic can fail Jul 28, 2021
@mattab mattab changed the title If no table prefix is not specified, the database abilities diagnostic can fail If table prefix is not specified, the database abilities diagnostic can fail Jul 28, 2021
@mattab mattab added the Bug For errors / faults / flaws / inconsistencies etc. label Jul 28, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@justinvelluppillai justinvelluppillai justinvelluppillai left review comments

@sgiehl sgiehl sgiehl approved these changes

Assignees
No one assigned
Labels
Bug For errors / faults / flaws / inconsistencies etc. Needs Review PRs that need a code review
Projects
None yet
Milestone
4.4.0
Development

Successfully merging this pull request may close these issues.

Problem with SQL table after migrating from Matomo 3 to Matomo 4
4 participants
@diosmosis @justinvelluppillai @sgiehl @mattab