serialization: restore initial behavior #8418
Conversation
WcaOnRails/app/models/competition.rb
Outdated
| @@ -1918,7 +1918,7 @@ def serializable_hash(options = nil) | |||
| # This looks weird, but we need the 'deeper_merge' method to handle arrays inside hashes. | |||
| # In turn, the 'deeper_merge' library has a quirk that even though it doesn't use the ! naming convention, | |||
| # it tries to modify the source array in-place. This is not cool so we need to circumvent by duplicating. | |||
There was a problem hiding this comment.
we need to circumvent by duplicating.
Is this comment still accurate? It looks like we removed the deep_dup?
There was a problem hiding this comment.
Whoops, I wasn't aware that we broke your dashboard! (Probably due to missing tests indeed, but I am also to blame for that for merging your PR without insisting you write tests 😅)
Your explanation about the purpose of serializable_hash totally makes sense and I think it should be applied to the API as well. In order to make the approach even more consistent, I've left two small comments
WcaOnRails/app/models/competition.rb
Outdated
| # This looks weird, but we need the 'deeper_merge' method to handle arrays inside hashes. | ||
| # In turn, the 'deeper_merge' library has a quirk that even though it doesn't use the ! naming convention, | ||
| # it tries to modify the source array in-place. This is not cool so we need to circumvent by duplicating. |
There was a problem hiding this comment.
This comment is now redundant again and can be deleted along with the deep_merge gem.
| only: %w[id name website start_date registration_open registration_close | ||
| announced_at cancelled_at end_date competitor_limit | ||
| extra_registration_requirements enable_donations refund_policy_limit_date event_change_deadline_date waiting_list_deadline_date on_the_spot_registration on_the_spot_entry_fee_lowest_denomination qualification_results | ||
| event_restrictions base_entry_fee_lowest_denomination currency_code allow_registration_edits allow_registration_self_delete_after_acceptance allow_registration_without_qualification refund_policy_percent | ||
| use_wca_registration guests_per_registration_limit venue contact force_comment_in_registration use_wca_registration external_registration_page guests_entry_fee_lowest_denomination guest_entry_status information], | ||
| methods: %w[registration_opened? main_event_id number_of_bookmarks using_stripe_payments? uses_qualification? uses_cutoff?], | ||
| include: %w[tabs], | ||
| methods: %w[url website short_name city venue_address venue_details latitude_degrees | ||
| longitude_degrees country_iso2 event_ids | ||
| registration_opened? main_event_id number_of_bookmarks using_stripe_payments? uses_qualification? uses_cutoff?], | ||
| include: %w[delegates organizers tabs], |
There was a problem hiding this comment.
If you add all the fields/methods/includes for the competition controller here, then you might as well delete the humongous COMPETITION_INFO_SERIALIZE_OPTIONS constant from the API competitions controller
viroulep
force-pushed
the
fix-deeper-merge
branch
from
6a79817
to
8681a74
Compare
This also explicitly lists all expected attributes when "showing" a competition's json.
viroulep
force-pushed
the
fix-deeper-merge
branch
from
8681a74
to
8c0181e
Compare
|
Thank you both for your quick reviews! I've fixed the comment and taken the liberty to add an explanation about why it's implemented like that, and I also added a commit adding a basic post/get test for the posting check in. |
viroulep
force-pushed
the
fix-deeper-merge
branch
from
8c0181e
to
c50fe36
Compare
|
Changes are online! https://www.worldcubeassociation.org/server-status |
This attempts to fix serialization for the results posting check-in, which was broken in #8409.
Gregor saw it coming:
We do pass a hash in the include part here ^^
It means that the
includepart is not merged at all and it broke the results check in (my bad for not testing the endpoint I think).I read a bit through #8409 and the initial implementation of
serializable_hashwas correct: the behavior I intended in #7199, is not to do a deep merge, but to be able to simply overrideonly,methods, andincludeto be more selective when creating hashes, and to be able to not include all the default options if you don't need them, which I believe is the purpose ofserializable_hashoptions.Some example of what I think the options should be used for:
onlyfield while passing an emptymethodsbecause we don't need themonlyfield by letting the caller pick exactly the field they wantI think this behavior is necessary when it comes to generate smaller json-s for objects than the default one (for instance now the json for the result "check in" is relatively big compared to the data we actually need).
I think an appropriate fix is to make the api competition controller explicit about the field it expects, rather than implementing a deep merge which prevent being selective about attributes we want to include in the serialization.
Here I explicitly set the attributes so that it's clear from just reading the endpoint what attributes end up in the json, but it would also be possible to use
Competitions::DEFAULT_SERIALIZABLE_OPTIONS[:...]if you feel it's more appropriate.