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

[chain filters] should not be using inactive fields for joins #39635

Merged
merged 3 commits into from
Mar 8, 2024

Conversation

piranha
Copy link
Contributor

@piranha piranha commented Mar 5, 2024

Resolves #39618

@piranha piranha added the backport Automatically create PR on current release branch on merge label Mar 5, 2024
@piranha piranha requested a review from a team March 5, 2024 15:02
@piranha piranha self-assigned this Mar 5, 2024
@piranha piranha requested a review from camsaul as a code owner March 5, 2024 15:02
@metabase-bot metabase-bot bot added the .Team/BackendComponents also known as BEC label Mar 5, 2024
@piranha piranha removed the request for review from camsaul March 5, 2024 15:10
Copy link

replay-io bot commented Mar 5, 2024

Status Complete ↗︎
Commit 7cfe353
Results
⚠️ 4 Flaky
2337 Passed

Copy link
Contributor

@crisptrutski crisptrutski left a comment

Choose a reason for hiding this comment

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

Makes sense to me. A pity about the manual reset, but doesn't seem worth rabbit holing for a small bug fix.

src/metabase/db/connection.clj Outdated Show resolved Hide resolved
src/metabase/db/connection.clj Outdated Show resolved Hide resolved
Copy link
Contributor

@calherries calherries left a comment

Choose a reason for hiding this comment

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

This PR seems to be fixing a symptom of the issue, not the root cause.

The fact that a field referenced by fk_target_field_id is inactive is real issue here. That state should be impossible. I thought I fixed that in Remove a retired field's FK target references during sync. It's possible that there was some edge case that I missed. If so we'll need to do another migration like this.

This fix is unfortunately too narrow because it fixes one symptom of the bug but it means there's probably other ways this becomes an issue out there. Having to check that a field is active every time we use fk_target_field_id is error prone and complicates the code everywhere we use this information.

@piranha
Copy link
Contributor Author

piranha commented Mar 6, 2024

@calherries hmm... I'm not understanding semantics completely, but is a field being inactive an error at all times?

@calherries
Copy link
Contributor

No, but a foreign key reference to an inactive field is definitely a bug, because the constraint doesn't exist in the database. This is a special case of the bug I raised yesterday #39687 (total coincidence though)

@piranha piranha closed this Mar 7, 2024
@calherries
Copy link
Contributor

Opening this again because I feel we should just land this rather than talking about solutions more.

@calherries calherries reopened this Mar 8, 2024
Copy link
Contributor

@calherries calherries left a comment

Choose a reason for hiding this comment

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

Looks good

@piranha piranha enabled auto-merge (squash) March 8, 2024 11:59
@piranha piranha merged commit 77b649d into master Mar 8, 2024
106 checks passed
@piranha piranha deleted the chain-filter-inactive branch March 8, 2024 12:39
Copy link

github-actions bot commented Mar 8, 2024

@piranha Did you forget to add a milestone to the issue for this PR? When and where should I add a milestone?

@piranha
Copy link
Contributor Author

piranha commented Mar 8, 2024

@metabase-bot backport release-x.48.x

metabase-bot bot added a commit that referenced this pull request Mar 8, 2024
#39847)

Co-authored-by: Alexander Solovyov <alexander@solovyov.net>
metabase-bot bot added a commit that referenced this pull request Mar 8, 2024
#39849)

Co-authored-by: Alexander Solovyov <alexander@solovyov.net>
@calherries calherries added this to the 0.49 milestone Mar 14, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport Automatically create PR on current release branch on merge .Team/BackendComponents also known as BEC
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Chain filters do not respect inactive fields
3 participants