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 statesman index detection #354

Merged

Conversation

hmarr
Copy link
Contributor

@hmarr hmarr commented Jul 4, 2019

The previous logic worked fine with parent tables that (approximately) began with the letters A-S, but with tables that (approximately) started with the letters T-Z, the comparison would fail, which ultimately resulted in the RecordNotUnique exception not being converted to a TransitionConflictError. Among other things, this prevents retry_conflicts from working.

@hmarr
Copy link
Contributor Author

hmarr commented Jul 4, 2019

Looks like the mongo tests are failing due to rails/rails#35153 (unrelated), and have been failing consistently for the past few builds.

Copy link
Contributor

@lawrencejones lawrencejones left a comment

Choose a reason for hiding this comment

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

Can we add a quick test that catches this? I can see that .sort going missing in a refactor later.

Might also be worth a small comment on the comparison that says we don't care about the index columns ordering as we only care about the uniqueness property? At first this looked wrong to me as I'm so used to index column ordering being integral to the index behaviour, but of course this doesn't matter when we just care about uniqueness.

Nice catch though!

@hmarr
Copy link
Contributor Author

hmarr commented Jul 5, 2019

Adding a test is a little tricky as the column name comes from the test model we use everywhere (MyActiveRecordModel). I did briefly consider renaming it to ZyActiveRecordModel but figured that'd confuse a lot of people 🤠 If you have any ideas on how we can easily test this without lots of stubbing and testing of private methods, I'm all ears! But it'd be great to get this fix in soon as we're seeing a lot of exceptions because of it :-)

Good shout on the comment - added!

Copy link
Contributor

@lawrencejones lawrencejones left a comment

Choose a reason for hiding this comment

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

If it's not easy to add in a sensible way then let's get this merged. It's not like we're unclear on the nature of the issue or uncertain that this will fix it.

If you can rebase then I can merge and cut a new release?

hmarr added 2 commits July 5, 2019 12:57
The previous logic worked fine with parent tables that began with the
letters A-S, but with tables that started with the letters T-Z, the
comparison would fail, which ultimately resulted in the RecordNotUnique
exception not being converted to a TransitionConflictError. Among other
things, this prevents `retry_conflicts` from working.
@hmarr hmarr force-pushed the fix-mysql-statesman-index-detection branch from 4e0bf27 to 5a6a9b4 Compare July 5, 2019 17:57
@hmarr
Copy link
Contributor Author

hmarr commented Jul 5, 2019

Thanks 🙌

Just rebased and pushed.

@lawrencejones lawrencejones merged commit 1a619c8 into gocardless:master Jul 6, 2019
@lawrencejones
Copy link
Contributor

This should now be released in v4.1.1

@hmarr
Copy link
Contributor Author

hmarr commented Jul 6, 2019

Thanks Lawrence!

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