Uh oh!
There was an error while loading. Please reload this page.
Add logging and remove validation with developer profile querying - #488
Conversation
bryceitoc9
left a comment
There was a problem hiding this comment.
Minor nit in removing some of the validity checking.
| private boolean isValidAccountId(final String accountId) { | ||
| return accountId != null && accountId.matches("^\\d{12}$"); | ||
| return StringUtils.isNotBlank(accountId); | ||
| } | ||
| private boolean isValidArn(final String arn) { | ||
| return arn != null && arn.matches("^arn:aws:codewhisperer:[a-z]{2}-[a-z]+-\\d:\\d{12}:profile/[A-Z0-9]+$"); | ||
| return StringUtils.isNotBlank(arn); | ||
| } | ||
| private boolean isValidRegion(final String region) { | ||
| return region != null && region | ||
| .matches("^[a-z]{2}-(central|north|south|east|west|northeast|southeast|northwest|southwest)-(\\d)$"); | ||
| return StringUtils.isNotBlank(region); | ||
| } |
There was a problem hiding this comment.
None of the other IDEs do it and clients are not expected to. We expect validation to happen on the backend side where an incorrect value should fail with an appropriate message. Also the validity checking was too regex heavy which was leading to invalid states happening on the Eclipse side.
There was a problem hiding this comment.
IMO it's fine to not have these check, in most scenarios, users won't touch / modify the cached file (also that's what we expect). We just need to ensure serialize/de-serialize work fine then it should be good enough.
| @JsonIgnore | ||
| public final String getRegion() { | ||
| return identityDetails.region(); | ||
| return identityDetails != null ? identityDetails.region() : null; |
There was a problem hiding this comment.
nit: iirc, this field is mostly null, should be more reliable to get region from the arn
https://github.com/aws/aws-toolkit-jetbrains/blob/main/plugins/amazonq/shared/jetbrains-community/src/software/aws/toolkits/jetbrains/services/amazonq/profile/QRegionProfile.kt#L12-L16
Uh oh!
There was an error while loading. Please reload this page.
Description of changes:
There is not sufficient logging around the developer profile fetching logic in the login flow. This change adds more logging. In addition, unnecessary restrictive validation checks on the stored and returned Q developer profiles has been removed as we want to rely on the backend to give us valid results. Only validation to check the shape of the stored value is now used, if it is incorrect, we expect further calls to the backend to fail with an appropriate message.
Related issue: #485
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.