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: Fixed Transforming wrapped input fields #4676

Merged
merged 5 commits into from
Aug 26, 2022
Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 6 additions & 1 deletion packages/wrap/src/transforms/TransformInputObjectFields.ts
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,8 @@ import {
OperationDefinitionNode,
isInputType,
NamedTypeNode,
isWrappingType,
isInputObjectType,
} from 'graphql';

import { Maybe, ExecutionRequest, MapperKind, mapSchema, transformInputValue } from '@graphql-tools/utils';
Expand Down Expand Up @@ -165,7 +167,10 @@ export default class TransformInputObjectFields<TContext = Record<string, any>>
// The casting is kind of legit here as we are in a visitor
const parentType = typeInfo.getInputType() as Maybe<GraphQLInputObjectType>;
Copy link
Owner

Choose a reason for hiding this comment

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

We shouldn't do casting here if we have the following check right?

Copy link
Contributor Author

@NullScope NullScope Aug 26, 2022

Choose a reason for hiding this comment

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

getInputType returns a GraphQLInputType instead of GraphQLInputObjectType, and does not have the property name. So on the ternary operator if the check fails it falls back to the original logic of just parentType.name. So I am not sure how we could do this without casting it here

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed with the getNamedType change

if (parentType != null) {
const parentTypeName = parentType.name;
const parentTypeName =
isWrappingType(parentType) && isInputObjectType(parentType.ofType)
Copy link
Owner

Choose a reason for hiding this comment

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

Related https://github.com/ardatan/graphql-tools/pull/4676/files#r956002686
Can we do getNamedType here because ofType can be also another wrapped type, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Well spotted! you can indeed have an "infinite" amount of wraps, will change it

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done!

Copy link
Owner

Choose a reason for hiding this comment

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

Awesome! Then could you create a changeset using yarn changeset then we can merge and cut a release?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done! Please check if the changeset makes sense 👍

? parentType.ofType.name
: parentType.name;
const newInputFields: Array<ObjectFieldNode> = [];

for (const inputField of node.fields) {
Expand Down
56 changes: 56 additions & 0 deletions packages/wrap/tests/transformRenameInputObjectFields.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -60,6 +60,62 @@ describe('RenameInputObjectFields', () => {
expect(testData.field2).toBe('field2');
});

test('renaming with non-null arguments works', async () => {
const schema = makeExecutableSchema({
typeDefs: /* GraphQL */ `
input InputObject {
field1: String
field2: String
}

type OutputObject {
field1: String
field2: String
}

type Query {
test(argument: InputObject!): OutputObject
}
`,
resolvers: {
Query: {
test: (_root, args) => {
return args.argument;
},
},
},
});

const transformedSchema = wrapSchema({
schema,
transforms: [
new RenameInputObjectFields((typeName, fieldName) => {
if (typeName === 'InputObject' && fieldName === 'field2') {
return 'field3';
}
}),
],
});

const query = /* GraphQL */ `
{
test(argument: { field1: "field1", field3: "field2" }) {
field1
field2
}
}
`;

const result = await execute({
schema: transformedSchema,
document: parse(query),
});
assertSome(result.data);
const testData: any = result.data['test'];
expect(testData.field1).toBe('field1');
expect(testData.field2).toBe('field2');
});

test('renaming with variables works', async () => {
const schema = makeExecutableSchema({
typeDefs: /* GraphQL */ `
Expand Down