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 empty response when hitting a temporary IA delay #2745

Merged
merged 1 commit into from
Mar 13, 2023

Conversation

mwiencek
Copy link
Member

_detach_with_temporary_delay incorrectly invokes detach_with_error by not wrapping the error key/value pairs in a hash ref. In Perl, => is the same as a comma except it auto-quotes bare words on the LHS. So the result was that 'message' was being passed as $error, and the l(...) string as $status. This malformed $status value triggers a warning:

Argument "We\x{2019}ve hit a temporary del..." isn't numeric in numeric lt (<)
  at /home/musicbrainz/carton-local/lib/perl5/Catalyst/Plugin/Cache/HTTP.pm line 228.
Argument "We\x{2019}ve hit a temporary del..." isn't numeric in numeric ge (>=)
  at /home/musicbrainz/carton-local/lib/perl5/Plack/Middleware/FixMissingBodyInRedirect.pm line 18.

...and causes MBS to return an empty response and a 502 on the gateway server.

`_detach_with_temporary_delay` incorrectly invokes `detach_with_error`
by not wrapping the error key/value pairs in a hash ref.  In Perl, `=>`
is the same as a comma except it auto-quotes bare words on the LHS.  So
the result was that `'message'` was being passed as `$error`, and the
`l(...)` string as `$status`.  This malformed `$status` value triggers a
warning:

```
Argument "We\x{2019}ve hit a temporary del..." isn't numeric in numeric lt (<)
  at /home/musicbrainz/carton-local/lib/perl5/Catalyst/Plugin/Cache/HTTP.pm line 228.
Argument "We\x{2019}ve hit a temporary del..." isn't numeric in numeric ge (>=)
  at /home/musicbrainz/carton-local/lib/perl5/Plack/Middleware/FixMissingBodyInRedirect.pm line 18.
```

...and causes MBS to return an empty response and a 502 on the gateway
server.
@reosarevok
Copy link
Member

@brainzbot, retest this please

Copy link
Member

@reosarevok reosarevok left a comment

Choose a reason for hiding this comment

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

The correction seems fine. We have plenty of detach_with_error calls in the form detach_with_error($c, 'string', XXX) too, maybe we should consolidate them to avoid issues like this one coming up by mistake again?

@mwiencek
Copy link
Member Author

mwiencek commented Dec 11, 2022

The correction seems fine. We have plenty of detach_with_error calls in the form detach_with_error($c, 'string', XXX) too, maybe we should consolidate them to avoid issues like this one coming up by mistake again?

Hmm, well not all of the calls can pass only a string, since at least one appears to include an error_details prop alongside the message.

@reosarevok
Copy link
Member

Yes, I meant the other way around, consistently using a props object rather than just a string :)

@mwiencek
Copy link
Member Author

I can change it for consistency, though not sure how much it'll help to prevent the same mistake. It'll just cause an exception (trying to de-reference a string) instead of a warning, which I guess could make the error more obvious. :)

@reosarevok
Copy link
Member

Crashing sounds a lot more clear than warning to me :D

@mwiencek mwiencek merged commit 74f7425 into metabrainz:master Mar 13, 2023
@mwiencek mwiencek deleted the fix-ia-delay-empty-response branch March 13, 2023 19:45
reosarevok added a commit that referenced this pull request Mar 13, 2023
* master:
  Update POT files using the production database
  Update translations from Transifex
  MBS-12902: Add pagination to admin email/IP search (#2862)
  Fix empty response when hitting a temporary IA delay (#2745)
  MBS-12959: Only hide My vote in header for non-voting editors (#2880)
  Use diag() rather than comments for email-search cases
  MBS-12994: Treat email as case-insensitive for admin email search
  MBS-12984: Link zoomable image in the WikiDocs (#2894)
  Update 45cat URL in tests after MBS-12721
  Drop 45cat/45worlds href override after MBBE-65
  Drop LiveFans href override after MBBE-73
  Drop OC ReMix href override after MBBE-74
  Drop Weibo href override after MBBE-75
  Regenerate cpanfile.snapshot files after MBS-12983
  Drop Recochoku href override after MBBE-38
  MBS-12980 (addendum): Transiently replace protocol
  MBS-12983: Update to DateTime::Locale 1.37
  MBS-12980: Use HTTPS for DNB permalinks
  MBS-12978: Widen the DNB regex even further
  Change React.Portal -> React$Portal
  Change React.AbstractComponent -> React$AbstractComponent
  Change React.MixedElement -> React$MixedElement
  Change React.Node -> React$Node
  Change React.Element -> React$Element
  Remove inference_mode=lti from .flowconfig
  Fix incorrect test description
  Remove some unnecessary uses of 'any'
  Remove unused $Flow comments
  Rename $Partial to Partial
  Upgrade Flow to 0.201.0
  Upgrade Flow to 0.200.1
  Upgrade Flow to 0.200.0
  Upgrade Flow to 0.199.1
  Upgrade Flow to 0.199.0
  Upgrade Flow to 0.198.2
  Upgrade Flow to 0.198.1
  Upgrade Flow to 0.198.0
  .flowconfig: enable inference_mode=lti
  Split Entity::AliasType into per-entity classes
  Add missing type types to linkedEntities
  Get rid of YAML format warnings
  Allow for JSX in .mjs files
  Refactor: Fix Flow type’s name for pending edits
  Refactor: Fix Data role’s name for pending edits
  Refactor: Fix Entity role’s name for pending edits
  Amend d83903c: refine type through Flow comment
  Rename CollectableCoreEntityT to CollectableEntityT
  Fine-grain entity types with aliases
  Fine-grain entity type for artist credit list
  Enforce collectable entity types where possible
  Fine-grain entity types with artist credits
  Allow specifying which resources an extra inc is for
  Follow Perl::Critic::Policy::Variables::ProhibitPunctuationVars
  Follow Perl::Critic::Policy::Variables::ProhibitConditionalDeclarations
  Refactor Server::Controller for clarity
  Refactor Data::Search for clarity / speed
  Follow Perl::Critic::Policy::Variables::ProhibitAugmentedAssignmentInDeclaration
  Follow Perl::Critic::Policy::Variables::ProtectPrivateVars
  Follow Perl::Critic::Policy::Variables::RequireNegativeIndices
  MBS-12962: Don't ISE on undef track_offset (#2882)
  MBS-11565: Use less hopeful image not available message (#2872)
  Upgrade Flow to 0.197.0
  Upgrade Flow to 0.196.3
  Upgrade Flow to 0.196.2
  Upgrade Flow to 0.196.1
  Remove some unused Flow suppressions
  Upgrade Flow to 0.196.0
  Upgrade Flow to 0.195.2
  Upgrade Flow to 0.195.1
  Upgrade Flow to 0.195.0
  .flowconfig: enable unused-promise-in-async-scope lint
  Upgrade Flow to 0.194.0
  Upgrade Flow to 0.193.0
  Upgrade Flow to 0.192.0
  Upgrade Flow to 0.191.0
  MBS-12955: Display beginner flag in vote tally
  Don't duplicate EditorTypeInfo
  MBS-12848: Reject inc=various-artists without inc=releases
reosarevok added a commit that referenced this pull request Mar 20, 2023
* beta:
  Update translations from Transifex
  Update POT files using the production database
  Update translations from Transifex
  MBS-12990: Don't pad the legend relationship editor fieldset (#2903)
  MBS-11786: Add a text view of statistics events
  MBS-11393: Show SetTrackLengths edit in recording edit history (#1969)
  MBS-12976: Changing a work can cause duplication/key errors
  batchSelectionCount should be undefined if unused
  Sync MediumT with Entity::Medium::TO_JSON
  Fix incorrect EntityRoleT type argument
  MBS-13001: Actually set source_type and target_type for historic rels (#2900)
  Load Entity:::*AliasType classes from Data::*AliasType
  Update POT files using the production database
  Update translations from Transifex
  MBS-12902: Add pagination to admin email/IP search (#2862)
  Fix empty response when hitting a temporary IA delay (#2745)
  Autocomplete2: Change useLayoutEffect to useEffect
  Actually submit the collection form in test
  CollectionEditForm: Use Autocomplete2 for collaborators
  MBS-12614: Better placement for Add collaborator button
  MBS-12959: Only hide My vote in header for non-voting editors (#2880)
  Use diag() rather than comments for email-search cases
  MBS-12994: Treat email as case-insensitive for admin email search
  MBS-12984: Link zoomable image in the WikiDocs (#2894)
  Update 45cat URL in tests after MBS-12721
  Drop 45cat/45worlds href override after MBBE-65
  Drop LiveFans href override after MBBE-73
  Drop OC ReMix href override after MBBE-74
  Drop Weibo href override after MBBE-75
  Regenerate cpanfile.snapshot files after MBS-12983
  Drop Recochoku href override after MBBE-38
  MBS-12980 (addendum): Transiently replace protocol
  MBS-12983: Update to DateTime::Locale 1.37
  MBS-12980: Use HTTPS for DNB permalinks
  MBS-12978: Widen the DNB regex even further
  Change React.Portal -> React$Portal
  Change React.AbstractComponent -> React$AbstractComponent
  Change React.MixedElement -> React$MixedElement
  Change React.Node -> React$Node
  Change React.Element -> React$Element
  Remove inference_mode=lti from .flowconfig
  Fix incorrect test description
  Remove some unnecessary uses of 'any'
  Remove unused $Flow comments
  Rename $Partial to Partial
  Upgrade Flow to 0.201.0
  Upgrade Flow to 0.200.1
  Upgrade Flow to 0.200.0
  Upgrade Flow to 0.199.1
  Upgrade Flow to 0.199.0
  Upgrade Flow to 0.198.2
  Upgrade Flow to 0.198.1
  Upgrade Flow to 0.198.0
  .flowconfig: enable inference_mode=lti
  Split Entity::AliasType into per-entity classes
  Add missing type types to linkedEntities
  Get rid of YAML format warnings
  Allow for JSX in .mjs files
  Refactor: Fix Flow type’s name for pending edits
  Refactor: Fix Data role’s name for pending edits
  Refactor: Fix Entity role’s name for pending edits
  Amend d83903c: refine type through Flow comment
  Rename CollectableCoreEntityT to CollectableEntityT
  Fine-grain entity types with aliases
  Fine-grain entity type for artist credit list
  Enforce collectable entity types where possible
  Fine-grain entity types with artist credits
  Allow specifying which resources an extra inc is for
  Follow Perl::Critic::Policy::Variables::ProhibitPunctuationVars
  Follow Perl::Critic::Policy::Variables::ProhibitConditionalDeclarations
  Refactor Server::Controller for clarity
  Refactor Data::Search for clarity / speed
  Follow Perl::Critic::Policy::Variables::ProhibitAugmentedAssignmentInDeclaration
  Follow Perl::Critic::Policy::Variables::ProtectPrivateVars
  Follow Perl::Critic::Policy::Variables::RequireNegativeIndices
  MBS-12962: Don't ISE on undef track_offset (#2882)
  MBS-11565: Use less hopeful image not available message (#2872)
  Upgrade Flow to 0.197.0
  Upgrade Flow to 0.196.3
  Upgrade Flow to 0.196.2
  Upgrade Flow to 0.196.1
  Remove some unused Flow suppressions
  Upgrade Flow to 0.196.0
  Upgrade Flow to 0.195.2
  Upgrade Flow to 0.195.1
  Upgrade Flow to 0.195.0
  .flowconfig: enable unused-promise-in-async-scope lint
  Upgrade Flow to 0.194.0
  Upgrade Flow to 0.193.0
  Upgrade Flow to 0.192.0
  Upgrade Flow to 0.191.0
  MBS-12955: Display beginner flag in vote tally
  Don't duplicate EditorTypeInfo
  MBS-12848: Reject inc=various-artists without inc=releases
  Move statistics event types to the types folder
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants