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

add limit to custom field ordering #7886

Conversation

3 participants
@angusmcleod
Copy link
Contributor

commented Jul 12, 2019

While not likely, it is possible to end up with two rows in a custom field table with the same related id.

See further: https://meta.discourse.org/t/custom-field-casting-affected-by-recent-update/122746

For example: https://discourse.angusmcleod.com.au/t/events-code-error-500/1254/4

The data will look something like this.

topic_custom_fields

id topic_id name value
9180 62 event_start 1563100201
9179 62 event_start 1563100201

In most respects this eventuality can be handled in a plugin, however there is one place where this causes a cardinality issue that can't be handled in a plugin: ordering by the custom field in a TopicQuery.

This PR adds a LIMIT to the custom field ordering statement in TopicQuery to handle that eventuality.

@discoursebot

This comment has been minimized.

Copy link

commented Jul 12, 2019

You've signed the CLA, angusmcleod. Thank you! This pull request is ready for review.

@discoursebot

This comment has been minimized.

Copy link

commented Jul 12, 2019

This pull request has been mentioned on Discourse Meta. There might be relevant details there:

https://meta.discourse.org/t/custom-field-casting-affected-by-recent-update/122746/4

@discoursebot

This comment has been minimized.

Copy link

commented Jul 12, 2019

This pull request has been mentioned on Discourse Meta. There might be relevant details there:

https://meta.discourse.org/t/events-plugin-calendar/69776/485

@ZogStriP

This comment has been minimized.

Copy link
Member

commented Jul 12, 2019

I'm sorry, but adding this LIMIT 1 would just hide bad data. I'd rather it blow up so that one can properly fix the data than have it silently fail.

In your case, you should add a migration in your plugin to remove all these duplicate values that are making the custom fields returning an array when you only want one value.

@ZogStriP ZogStriP closed this Jul 12, 2019

@angusmcleod angusmcleod deleted the angusmcleod:add_limit_to_custom_field_ordering branch Jul 14, 2019

@angusmcleod

This comment has been minimized.

Copy link
Contributor Author

commented Jul 14, 2019

@ZogStriP I understand where you're coming from, but I'm not sure adding a data migration in a plugin is a good solution. If I follow that approach I'll need to add a migration whenever I use custom_field ordering in TopicQuery, to guard against it.

Moreover it would still cause my users' sites to crash whenever the issue rears its head again (i.e. in-between migrations). And the handling of custom field storage may change in the future, invalidating the assumptions the migration is written on.

I think the solution for me for now is to move ordering inside the plugin itself.

@discoursebot

This comment has been minimized.

Copy link

commented Jul 14, 2019

This pull request has been mentioned on Discourse Meta. There might be relevant details there:

https://meta.discourse.org/t/events-plugin-calendar/69776/489

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.