[UI] Ember Data Migration - API Property Casing#31325
Conversation
github-actions
Bot
added
the
hashicorp-contributed-pr
|
CI Results: |
|
Build Results: |
| ttl: authData?.lease_duration, | ||
| ttl: ttl || authData?.lease_duration, | ||
| policies: authData?.policies, | ||
| mfaRequirement: authData?.mfa_requirement, |
There was a problem hiding this comment.
I guess the major difference is previously I spread the auth response in the normalize function (at the top) and then overrode keys with the normalized names.
Mostly because I was concerned there could be additional data returned by the API's login response that I missed during the refactor and didn't have test coverage. For example, there's this num_uses key - I don't think it was used anywhere but wasn't sure if there just wasn't test coverage for it.
There was a problem hiding this comment.
Right ya I see what you are saying. Your approach was definitely more future proof and I think something that we could look at in a follow up.
The one issue I see with doing that now is that these properties that are explicitly defined are in camelCase and the remaining properties from the spread would now be snake_case which could be confusing.
Given the thorough test coverage (thanks Claire!) I feel pretty good that the existing functionality is preserved but we should definitely look at ways to continue to improve all of this once we have consistent key casing across the app for API props.
There was a problem hiding this comment.
The one issue I see with doing that now is that these properties that are explicitly defined are in camelCase and the remaining properties from the spread would now be snake_case which could be confusing.
☝️ yes exactly! I think that would be confusing.
Yep - sounds good! Thanks for thinking that through with me!
| "@babel/core": "7.26.10", | ||
| "@hashicorp/design-system-components": "4.18.2", | ||
| "@hashicorp/vault-client-typescript": "portal:./api-client", | ||
| "@hashicorp/vault-client-typescript": "hashicorp/vault-client-typescript", |
|
|
||
| switch (type) { | ||
| case 'aws': | ||
| return camelCase ? stringArrayToCamelCase(awsKeys) : awsKeys; |
hellobontempo
left a comment
hellobontempo
left a comment
There was a problem hiding this comment.
I think not spreading the auth response (and just pulling the mfa_requirement key explicitly) is probably okay. Mostly wanted to call it out to see if you had any thoughts about it.
Thanks for tackling this! Seeing these changes really confirms we made the right decision here 🎉
* updates api client vars to snake_case for custom messages * updates api client vars to snake_case for tools * updates api client vars to snake_case for sync * updates api client vars to snake_case for secrets engine * updates api client vars to snake_case for auth * updates api client vars to snake_case for usage * updates api client dep to point to gh repo * fixes custom-messages service unit tests * fixes configure-ssh test * fixes configure-ssh test...again
2 participants
Description
Originally the API client was set up to convert model and param name casing to camelCase. Since most of the references to Ember Data model attributes in the app used this casing, it made for an easier migration at the outset.
It was recently discovered that this poses a problem in certain cases where user defined values (paths for secrets engines or auth methods) are returned as keys in the response from the server and these keys were then being converted to camelCase by the client which incorrectly represents the original value if it contained underscores.
This wasn't a problem with Ember Data since the models contained both the original properties from the server (snake_cased) as well as camelCased versions of the attributes so either could be referenced as needed.
The API client has been updated to preserve the original casing returned from the server and this PR updates any references to camelCased API properties in the app.
TODO only if you're a HashiCorp employee
backport/label that matches the desired release branch. Note that in the CE repo, the latest release branch will look likebackport/x.x.x, but older release branches will bebackport/ent/x.x.x+ent.of a public function, even if that change is in a CE file, double check that
applying the patch for this PR to the ENT repo and running tests doesn't
break any tests. Sometimes ENT only tests rely on public functions in CE
files.
in the PR description, commit message, or branch name.
description. Also, make sure the changelog is in this PR, not in your ENT PR.
PCI review checklist
Examples of changes to security controls include using new access control methods, adding or removing logging pipelines, etc.