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 returning [Type, false] from resolve_type #4412

Merged
merged 1 commit into from Mar 29, 2023

Conversation

swalkinshaw
Copy link
Collaborator

@swalkinshaw swalkinshaw commented Mar 29, 2023

#4130 fixed returning [Type, nil] from a union's resolve_type, but it caused a regression for literal false values.

Schema#resolve_type had an if resolved_value conditional would be false for false values causing it always return a single value (the resolve_type).

The runtime's resolve_type method would then always return the tuple version (in non-lazy cases): [type, value]. However, since Schema#resolve_type was only returning a single value, the resolved value would be lost resulting in [type, nil] being passed on.

The fix is to remove the if resolved_value conditional entirely and always return the two-value tuple ensuring the original resolved value is correctly passed along.

Note: the runtime still has a branch to check if the resolve_type value is a tuple or not. The only case where this won't be true is for lazy resolved types.

The call stack is a bit confusing, so here's all the relevant code in call order:

  1. Schema#resolve_type

    if resolved_value
    [resolved_type, resolved_value]
    else
    resolved_type
    end

  2. Runtime#resolve_type

    [resolved_type, resolved_value]

  3. Runtime#continue_field

    if resolved_type_result.is_a?(Array) && resolved_type_result.length == 2
    resolved_type, resolved_value = resolved_type_result
    else
    resolved_type = resolved_type_result
    resolved_value = value
    end

cc @IDolgirev

#4130 fixed returning
`[Type, nil]` from a union's `resolve_type`, but it caused a regression
for literal `false` values.

`Schema#resolve_type` had a `if resolved_value` conditional would be false
for `false` values causing it _always_ return a single value (the `resolve_type`).

The runtime's `resolve_type` method would then _always_ return the tuple
version (in non-lazy cases): `[type, value]`. However, since
`Schema#resolve_type` was only returning a single value, the resolved
value would be lost resulting in `[type, nil]` being passed on.

The fix is to remove the `if resolved_value` conditional entirely and
always return the two-value tuple ensuring the original resolved value
is correctly passed along.

Note: the runtime still has a branch to check if the `resolve_type`
value is a tuple or not. The only case where this won't be true is for
lazy resolved types.
@swalkinshaw swalkinshaw force-pushed the fix-union-resolve-type-tuple-value branch from 25f26d3 to e755a35 Compare March 29, 2023 15:49
Copy link
Owner

@rmosolgo rmosolgo left a comment

Choose a reason for hiding this comment

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

Wow -- thanks for the fix and test on this!

@rmosolgo rmosolgo merged commit 05aded3 into master Mar 29, 2023
@rmosolgo rmosolgo deleted the fix-union-resolve-type-tuple-value branch March 29, 2023 16:16
@rmosolgo rmosolgo added this to the 2.0.20 milestone Mar 29, 2023
@swalkinshaw
Copy link
Collaborator Author

Thanks for the backport!

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.

None yet

2 participants