Skip to content

Type match incorrect for PATCH requests #789

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

Closed
bjornharrtell opened this issue Jul 7, 2020 · 7 comments · Fixed by #821
Closed

Type match incorrect for PATCH requests #789

bjornharrtell opened this issue Jul 7, 2020 · 7 comments · Fixed by #821
Assignees
Labels

Comments

@bjornharrtell
Copy link
Contributor

It seems logic is at fault at:

var deserializedType = context.ActionArguments.FirstOrDefault().Value?.GetType();
var targetType = context.ActionDescriptor.Parameters.FirstOrDefault()?.ParameterType;

For PATCH requests that will compare the path id parameter not the entity.

@bjornharrtell
Copy link
Contributor Author

LastOrDefault could possible be more logical but not sure it it's foolproof.

@bart-degreed
Copy link
Contributor

Hi @bjornharrtell, I'm not confident I understand what is going on here exactly. Can you write up a test that demonstrates the problem?

@maurei
Copy link
Member

maurei commented Sep 4, 2020

Closing due to inactivity. I tried reproducing a bug but I haven't been able to. Feel free to reply with new information and we'll check it out.

@maurei maurei closed this as completed Sep 4, 2020
@bart-degreed
Copy link
Contributor

Reopening, as I can reproduce the problem.
Both lines pick the first argument, while the info we need is at the second argument for both lines.

Debug the test Can_Update_ToMany_Relationship_By_Patching_Resource and add a breakpoint on these lines.
It is comparing Guid types, while it should be comparing TodoItemCollection types.

@bart-degreed bart-degreed reopened this Sep 8, 2020
@bart-degreed
Copy link
Contributor

OrDefault().Value is wrong anyway, it would immediately crash in case of default.

@bjornharrtell
Copy link
Contributor Author

@bart-degreed I can confirm that your observations are the same as mine.

@maurei
Copy link
Member

maurei commented Sep 9, 2020

Thanks for looking into this. I realise I was looking completely in the wrong direction for a reproduction.

LastOrDefault could possible be more logical but not sure it it's foolproof.

This is correct, but just Last, default values make no sense here. It does actually. The default value is a [null, null] key value pair, which happens for example when an empty relationship collection is patched to a ../relationships/.. endpoint

We also need to move away from inspecting ActionDescriptor parameters because this will lead to a comparison with Object type for the {id}/relationships/{relationshipName} endpoint, which will never match the runtime type in ActionArguments. See endpoint method signature.

We can resolve the expected type from IJsonApiRequest instead

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Development

Successfully merging a pull request may close this issue.

3 participants