-
Notifications
You must be signed in to change notification settings - Fork 77
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: enable IAM auth for custom types #2961
Conversation
858ea80
to
3563071
Compare
@@ -161,6 +161,22 @@ describe('Deprecate Gen 1 patterns', () => { | |||
); | |||
}); | |||
|
|||
test('does not allow implicit fields on @hasMany', () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This was an opportunistic test addition that I found while I was investigating how a customer-provided GraphQL schema is being mutated in other contexts.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Renamed this file from "defnition.test.ts", added the entire extendObjectWithDirectives
block. Didn't touch the gets Non-Model Types
block
/* eslint-disable prefer-arrow/prefer-arrow-functions */ | ||
/* eslint-disable func-style */ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Makes this file easier to read in vscode since it suppresses the red squiggly eslint errors that would otherwise decorate the entire file.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Meanwhile with Neovim's LSP capabilities I apparently live in blissful ignorance.
export const hasDirectiveWithName = ( | ||
node: FieldDefinitionNode | InterfaceTypeDefinitionNode | ObjectTypeDefinitionNode, | ||
name: string, | ||
): boolean => { | ||
return node.directives?.some((d) => d.name.value === name) ?? false; | ||
}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Possible duplicate/extension of code here:
export const directiveExists = (definition: ObjectTypeDefinitionNode, name: string) => {
return definition?.directives?.find((directive) => directive?.name?.value === name);
};
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, thanks for finding that. I'll fix in a followup
const newDirectives = []; | ||
|
||
for (const directive of directives) { | ||
if (!object.directives.some((d) => d.name.value === directive.name.value)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Optionally could use hasDirectiveWithName
but it also matches the rest of the functions in this file as is.
/* eslint-disable prefer-arrow/prefer-arrow-functions */ | ||
/* eslint-disable func-style */ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Meanwhile with Neovim's LSP capabilities I apparently live in blissful ignorance.
packages/amplify-graphql-auth-transformer/src/graphql-auth-transformer.ts
Outdated
Show resolved
Hide resolved
packages/amplify-graphql-auth-transformer/src/graphql-auth-transformer.ts
Outdated
Show resolved
Hide resolved
packages/amplify-graphql-auth-transformer/src/graphql-auth-transformer.ts
Outdated
Show resolved
Hide resolved
export const addDirectivesToObject = ( | ||
ctx: TransformerTransformSchemaStepContextProvider, | ||
typeName: string, | ||
directives: Array<DirectiveNode>, | ||
): void => { | ||
const type = ctx.output.getType(typeName) as ObjectTypeDefinitionNode; | ||
if (type) { | ||
ctx.output.putType(extendObjectWithDirectives(type, directives)); | ||
} | ||
}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: This would break convention in this file...
export const addDirectivesToObject = ( | |
ctx: TransformerTransformSchemaStepContextProvider, | |
typeName: string, | |
directives: Array<DirectiveNode>, | |
): void => { | |
const type = ctx.output.getType(typeName) as ObjectTypeDefinitionNode; | |
if (type) { | |
ctx.output.putType(extendObjectWithDirectives(type, directives)); | |
} | |
}; | |
export const addDirectivesToObject = ( | |
ctx: TransformerTransformSchemaStepContextProvider, | |
typeName: string, | |
directives: Array<DirectiveNode>, | |
): void => { | |
const obj = ctx.output.getObject(typeName); | |
if (obj) { | |
ctx.output.updateObject(extendObjectWithDirectives(obj, directives)); | |
} | |
}; |
Reverted this b/c the manual test failed during deployment. Turns out that it was a different issue unrelated to this change, but we'll re-implement the change alongside an E2E, in a separate PR. |
Description of changes
Enables IAM authorization for custom types.
Issue #, if available
#2929
Checklist
yarn test
passesBy submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.