Skip to content

[FileUpload] Allow to encode custom scalar types as null and use it for file uploads #2566

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 3 commits into from
Sep 17, 2020
Merged

[FileUpload] Allow to encode custom scalar types as null and use it for file uploads #2566

merged 3 commits into from
Sep 17, 2020

Conversation

@martinbonnin
Copy link
Contributor

fixes #2409

@martinbonnin
Copy link
Contributor Author

Ping @meiblorn. Sorry for the delay getting this in, let me know if this looks good for you.

@vsfedorenko
Copy link

@martinbonnin, thanks a lot. I haven't tested it yet, but overall it looks good.

sav007
sav007 reviewed Sep 13, 2020
View reviewed changes
...api/src/commonMain/kotlin/com/apollographql/apollo/api/internal/json/InputFieldJsonWriter.kt
is GraphQLString -> writeString(fieldName, customTypeValue.value)
is GraphQLBoolean -> writeBoolean(fieldName, customTypeValue.value)
is GraphQLNumber -> writeNumber(fieldName, customTypeValue.value)
is GraphQLNull -> writeString(fieldName, null)
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: should we introduce writeNull to our JsonWriter? 😄

Copy link
Contributor Author

Choose a reason for hiding this comment

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

JsonWriter already has nullValue, would that be ok?

We could also add writeNull to InputFieldWriter but that would be a much more invasive change that I'm not too keen on doing in 2.x. Falling into the rabbit hole of InputFieldsWriters, I realized that SortedInputFieldMapWriter doesn't handle custom scalar types. So custom scalars as field arguments will most likely fill the cache with different values. I guess something to keep in mind for 3.x

@martinbonnin martinbonnin mentioned this pull request Sep 14, 2020
Closed
@martinbonnin martinbonnin changed the base branch from master to main September 16, 2020 09:53
@martinbonnin martinbonnin changed the title Allow to encode custom scalar types as null and use it for file uploads [FileUpload] Allow to encode custom scalar types as null and use it for file uploads Sep 16, 2020
@martinbonnin martinbonnin merged commit a31a58c into main Sep 17, 2020
@martinbonnin martinbonnin deleted the bug-2409/send-null-in-file-upload branch September 17, 2020 07:08
@martinbonnin martinbonnin restored the bug-2409/send-null-in-file-upload branch September 17, 2020 12:10
@martinbonnin martinbonnin deleted the bug-2409/send-null-in-file-upload branch December 21, 2020 13:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@tasomaniac tasomaniac Awaiting requested review from tasomaniac

1 more reviewer

@sav007 sav007 sav007 approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

graphql-multipart-request-spec is not followed fully

3 participants

@martinbonnin @vsfedorenko @sav007