Skip to content

Conversation

@fangyuwu7
Copy link
Contributor

@fangyuwu7 fangyuwu7 commented Jul 10, 2024

Problem

userAttributes does not currently support adding custom attributes.

Issue number, if available:

Changes

Support adding custom attributes into userAttributes.

Corresponding docs PR, if applicable:

Validation

unit tests

Checklist

  • If this PR includes a functional change to the runtime behavior of the code, I have added or updated automated test coverage for this change.
  • If this PR requires a change to the Project Architecture README, I have included that update in this PR.
  • If this PR requires a docs update, I have linked to that docs PR above.
  • If this PR modifies E2E tests, makes changes to resource provisioning, or makes SDK calls, I have run the PR checks with the run-e2e label set.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

@fangyuwu7 fangyuwu7 requested a review from a team as a code owner July 10, 2024 23:48
@changeset-bot
Copy link

changeset-bot bot commented Jul 10, 2024

🦋 Changeset detected

Latest commit: 400d0bd

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
@aws-amplify/auth-construct Minor

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

'@aws-amplify/auth-construct': minor
---

adding customAttributes into userAttributes
Copy link
Contributor

Choose a reason for hiding this comment

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

phoneNumber: DEFAULTS.IS_REQUIRED_ATTRIBUTE.phoneNumber(phoneEnabled),
...(props.userAttributes ? props.userAttributes : {}),
...(props.userAttributes
? Object.entries(props.userAttributes).reduce(
Copy link
Contributor

Choose a reason for hiding this comment

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

Instead of looping Object.entries(props.userAttributes) twice on line 462 and line 475, we can loop it once and get two arrays.
Something like `{ userAttributes, customAttibutes } = Object.entries(props.userAttributes).reduce(...)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated.

Copy link
Contributor

@rtpascual rtpascual left a comment

Choose a reason for hiding this comment

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

Looks like this PR is missing changes to auth-construct/API.md, don't forget to run npm run update:api to fix the failing check_api_changes and check_api_extract checks.

/**
* Define bindCustomAttribute to meet requirements of the Cognito API to call the bind method
*/
public bindCustomAttribute = (
Copy link
Contributor

Choose a reason for hiding this comment

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

seems like this method should be private. We should test this functionality via constructor inputs. I see there already are some unit tests exercising this; maybe that is sufficient and we don't need the unit tests that are directly calling this method?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated.

/**
* CustomAttributes is a union type that represents all the different types of custom attributes.
*/
export type CustomAttributes =
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
export type CustomAttributes =
export type CustomAttribute =

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated.

@fangyuwu7 fangyuwu7 requested a review from a team as a code owner July 11, 2024 19:49
const config: CustomAttributeConfig = {
dataType: attribute.dataType,
mutable: attribute.mutable ?? true,
stringConstraints:
Copy link
Contributor

Choose a reason for hiding this comment

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

if attribute.dataType === 'Number', do we still want the undefined stringConstraints to be there?

If not, we should conditionally add stringConstraints and numberConstraints

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated.

Comment on lines 604 to 624
'custom:display_name': {
dataType: 'String',
mutable: true,
maxLen: 100,
minLen: 0,
},
'custom:tenant_id': {
dataType: 'Number',
mutable: false,
max: 66,
min: 1,
},
'custom:member_year': {
dataType: 'Number',
max: 90,
min: 0,
},
'custom:favorite_song': {
dataType: 'String',
mutable: true,
},
Copy link
Contributor

Choose a reason for hiding this comment

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

Add unit tests for boolean and dateTime as well.

Copy link
Contributor Author

@fangyuwu7 fangyuwu7 Jul 15, 2024

Choose a reason for hiding this comment

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

Updated.

// Conditionally add constraint properties based on dataType.
if (attribute.dataType === 'String') {
constraints = {
...constraints,
Copy link
Contributor

Choose a reason for hiding this comment

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

Does this serve any purpose if constraints was empty to begin with?

Copy link
Contributor Author

@fangyuwu7 fangyuwu7 Jul 15, 2024

Choose a reason for hiding this comment

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

Updated.

};
} else if (attribute.dataType === 'Number') {
constraints = {
...constraints,
Copy link
Contributor

Choose a reason for hiding this comment

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

Does this serve any purpose if constraints was empty to begin with?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated.

[key, value]
) => {
if (key.startsWith('custom:')) {
const attributeKey = key.replace(/^(custom:|User\.?)/i, '');
Copy link
Contributor

Choose a reason for hiding this comment

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

Why do we need User. match as well? I don't see corresponding unit tests either.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

After testing, actually we don't need User. match.

@fangyuwu7 fangyuwu7 closed this Jul 15, 2024
@fangyuwu7 fangyuwu7 reopened this Jul 15, 2024
@fangyuwu7 fangyuwu7 closed this Jul 15, 2024
@fangyuwu7 fangyuwu7 reopened this Jul 15, 2024
@fangyuwu7 fangyuwu7 merged commit 10955d9 into aws-amplify:main Jul 17, 2024
@josefaidt josefaidt linked an issue Jul 22, 2024 that may be closed by this pull request
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

native support for custom attributes on auth

5 participants