Skip to content

[#4169] UniqueInputFieldNamesRule incorrectly identifies field names as duplicates within and/or array #4170

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

Closed
wants to merge 6 commits into from
Closed

[#4169] UniqueInputFieldNamesRule incorrectly identifies field names as duplicates within and/or array #4170

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
9498a1d
able to duplicate field name in array
vctqs1 Aug 17, 2024
2eb4d83
able to duplicate field name in array
vctqs1 Aug 17, 2024
f21ba92
perform check if current is not error
vctqs1 Aug 17, 2024
b1b5ca5
using stack to hold the list value instead
vctqs1 Aug 18, 2024
48dc656
update test with value args
vctqs1 Aug 18, 2024
3706c2d
update test with value args
vctqs1 Aug 18, 2024
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
50 changes: 50 additions & 0 deletions src/validation/__tests__/UniqueInputFieldNamesRule-test.ts
View file
Open in desktop
Copy link
Member

Choose a reason for hiding this comment

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

I was checking your tests and all of these also pass in current 16.x.x so I don't think this shows any issue in the current shape

Copy link
Author

Choose a reason for hiding this comment

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

ah yeah, you're right. Sorry, I misread the format; the issue was with the {} inside the single big object (labeled in blue).
image

something like [{Id: {}, Id: {}}]

our tests cases was [{Id: {}}, {Id: {}}]

Copy link
Member

@JoviDeCroock JoviDeCroock Aug 18, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Yes but

  it('duplicate input object fields in objects of array', () => {
    expectErrors(`
      {
        field(arg: [{Id: {}, Id: {}}])
      }
    `).toDeepEqual([
      {
        message: 'There can be only one input field named "Id".',
        locations: [
          { line: 3, column: 22 },
          { line: 3, column: 30 },
        ],
      },
    ]);
  });

Correctly reports it as an error

Copy link
Author

Choose a reason for hiding this comment

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

Oh, based on that, I think the issue is with Salesforce rather than the current validation rule. Thank you for your response, thank you for spending time to help me validation again.

Copy link
Member

Choose a reason for hiding this comment

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

No worries, happy to help, if anything pops up feel free to reach out/create an issue so we can get to the bottom of this.

Copy link
Author

Choose a reason for hiding this comment

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

I get the error query from Salesforce docuemtn https://developer.salesforce.com/docs/platform/graphql/guide/filter-joins.html#multiple-semi-joins-or-anti-joins

Copy link
Author

Choose a reason for hiding this comment

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

thank you, I'll the close PR right now. @JoviDeCroock

Original file line number Diff line number Diff line change
Expand Up @@ -53,6 +53,56 @@ describe('Validate: Unique input field names', () => {
`);
});

it('allow and/or with duplicate fields in array', () => {
expectValid(`
{
field(arg: { and: [{ f: true }, { f: true }] })
}
`);
expectValid(`
{
field(arg: { or: [{ f: true }, { f: {f1: true} }] })
}
`);
expectValid(`
{
field(arg: { or: [{ f: true }, { f1: {f: true} }] })
}
`);
});

it('duplicate input object fields in objects of array', () => {
expectErrors(`
{
field(arg: { or: [{ f: true, f: true }] })
}
`).toDeepEqual([
{
message: 'There can be only one input field named "f".',
locations: [
{ line: 3, column: 29 },
{ line: 3, column: 38 },
],
},
]);
});

it('nested input object fields in objects of array', () => {
expectErrors(`
{
field(arg: { or: [{f2: true}, { f1: {f2: "value", f2: "value" }}] })
}
`).toDeepEqual([
{
message: 'There can be only one input field named "f2".',
locations: [
{ line: 3, column: 46 },
{ line: 3, column: 59 },
],
},
]);
});

it('duplicate input object fields', () => {
expectErrors(`
{
Expand Down
43 changes: 41 additions & 2 deletions src/validation/rules/UniqueInputFieldNamesRule.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@ import type { ObjMap } from '../../jsutils/ObjMap';

import { GraphQLError } from '../../error/GraphQLError';

import type { NameNode } from '../../language/ast';
import type { NameNode, ObjectFieldNode } from '../../language/ast';
import type { ASTVisitor } from '../../language/visitor';

import type { ASTValidationContext } from '../ValidationContext';
Expand All @@ -22,6 +22,9 @@ export function UniqueInputFieldNamesRule(
const knownNameStack: Array<ObjMap<NameNode>> = [];
let knownNames: ObjMap<NameNode> = Object.create(null);

let knownNamesInList: Array<ReadonlyArray<ObjectFieldNode>> =
Object.create([]);

return {
ObjectValue: {
enter() {
Expand All @@ -34,18 +37,54 @@ export function UniqueInputFieldNamesRule(
knownNames = prevKnownNames;
},
},
ListValue: {
enter(node) {
node.values.forEach((valueNode) => {
if(valueNode.kind === 'ObjectValue') {
knownNamesInList.push(valueNode.fields);
}
});
},
leave() {
knownNamesInList = Object.create([]);
Copy link
Member

Choose a reason for hiding this comment

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

I think this would also need to use a stack else it would make list --> object --> field --> list lose track of the outer object when it bubbles back up

Copy link
Author

Choose a reason for hiding this comment

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

Let me address the case of this list --> object --> field --> list.

Copy link
Author

Choose a reason for hiding this comment

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

I’ve changed the approach to use a stack to hold listValue[] and pop the state when it exits. I’ve also added a test for this case. Could you please help me review it again? @JoviDeCroock

},
},
ObjectField(node) {
const fieldName = node.name.value;

let isError = false;
if (knownNames[fieldName]) {
if (!knownNamesInList.length) {
isError = true;
}
else {
for (const fields of knownNamesInList) {
const nestedFields = fields.filter(
(field) => field.name.value === fieldName,
);

// expecting only one field with the same name, if there is more than one, report error. if there is no field with the same name, mean it is in the nested object instead list value, report error.
if (!isError && nestedFields.length !== 1) {
isError = true;
}
}
}
}



if(isError) {
context.reportError(
new GraphQLError(
`There can be only one input field named "${fieldName}".`,
{ nodes: [knownNames[fieldName], node.name] },
),
);
} else {
}
else {
knownNames[fieldName] = node.name;
}
},

};
}
Loading