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 rehydration of subscription args with differing internal representations #1415

Draft
wants to merge 16 commits into
base: master
Choose a base branch
from

Conversation

spawnia
Copy link
Collaborator

@spawnia spawnia commented Jun 11, 2020

  • Added or updated tests
  • Documented user facing changes
  • Updated CHANGELOG.md

Changes

Breaking changes

@spawnia
Copy link
Collaborator Author

spawnia commented Jun 11, 2020

@stayallive opened the PR to see the results of the test run.

@spawnia
Copy link
Collaborator Author

spawnia commented Jun 14, 2020

That's a tricky one.

We currently store the internal value of the ENUM when serializing the subscription, which causes the rehydrated execution during the broadcast to fail. This might also be problematic when using custom scalar types whose internal representation might not be serializable.

I think we should try and get a hold of the raw args/variables that are passed during the initial subscription request and serialize exactly those. That will ensure the rehydrated requests function exactly the same.

@spawnia spawnia added the bug An error within Lighthouse label Jun 14, 2020
@spawnia spawnia changed the title Add failing test for variables + subscriptions Fix rehydration of subscription args with differing internal representations Jun 14, 2020
@stayallive
Copy link
Collaborator

Yeah was thinking the same, will brew a bit on this, thanks for the pointers 👍

@spawnia spawnia marked this pull request as draft July 17, 2020 10:11
@stayallive
Copy link
Collaborator

That prettify docs shouldn't have ran here and or had any effect... that will mess up some PR's 😉

@spawnia
Copy link
Collaborator Author

spawnia commented Aug 27, 2020

@stayallive a tricky part is that the raw request may contain literal argument values as well as variables. We don't ever see an intermediary representation where those are merged, but not cast to their internal values (e.g. Enum classes).

I dug in to graphql-php and see if they expose this structure, or at least a method to build it from the raw request. Looks like there is no representation of raw, merged arguments: https://github.com/webonyx/graphql-php/blob/4f3430990824ff410fe548102cb85f0c46442704/src/Executor/ReferenceExecutor.php#L623-L627

spawnia and others added 5 commits January 24, 2024 17:53
# Conflicts:
#	src/Subscriptions/SubscriptionBroadcaster.php
#	src/Subscriptions/SubscriptionResolverProvider.php
#	tests/Integration/Subscriptions/SubscriptionTest.php
Comment on lines +155 to +162
public function parseValue($value)
{
if ($value instanceof $this->enumClass) {
return $value;
}

return parent::parseValue($value);
}
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Hey @stayallive, I think this might fix the issue at least for enums from bensampo/laravel-enum. I don't know what we can do about the enums modified by @enum tnough.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug An error within Lighthouse
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants