Skip to content
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

feat: Custom user agent enhancement for api-rest #11457

Conversation

stocaaro
Copy link
Member

@stocaaro stocaaro commented Jun 5, 2023

Description of changes

Custom user agent enhancement for api-rest

Description of how you validated changes

Test improvements to the API category.

This testing doesn't confirm inclusion in the client, which I will verify manually. Test augmented in api-rest to cover passing UserAgentDetails out to the client call header.

Checklist

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

Sorry, something went wrong.

@stocaaro stocaaro requested review from a team as code owners June 5, 2023 16:06
@@ -382,7 +400,10 @@
await api.get('apiName', '/items', init);
const expectedParams = {
data: null,
headers: { Authorization: 'apikey' },
headers: {
Authorization: 'apikey',

Check failure

Code scanning / CodeQL

Hard-coded credentials

The hard-coded value "apikey" is used as [authorization header](1).
@codecov-commenter
Copy link

Codecov Report

Merging #11457 (30a4cc0) into feat/user-agent-enhancements/main (d295bd1) will increase coverage by 0.01%.
The diff coverage is 100.00%.

❗ Your organization is not using the GitHub App Integration. As a result you may experience degraded service beginning May 15th. Please install the Github App Integration for your organization. Read more.

@@                          Coverage Diff                          @@
##           feat/user-agent-enhancements/main   #11457      +/-   ##
=====================================================================
+ Coverage                              83.14%   83.15%   +0.01%     
=====================================================================
  Files                                    260      260              
  Lines                                  20429    20437       +8     
  Branches                                4402     4400       -2     
=====================================================================
+ Hits                                   16985    16995      +10     
+ Misses                                  3158     3156       -2     
  Partials                                 286      286              
Impacted Files Coverage Δ
packages/api-rest/src/RestClient.ts 95.23% <100.00%> (+1.23%) ⬆️
packages/api/src/internals/InternalAPI.ts 97.05% <100.00%> (+0.23%) ⬆️
packages/core/src/Platform/types.ts 100.00% <100.00%> (ø)

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

Copy link
Contributor

@erinleigh90 erinleigh90 left a comment

Choose a reason for hiding this comment

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

LGTM!

Copy link
Member

@svidgen svidgen left a comment

Choose a reason for hiding this comment

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

No blockers for me.

Comment on lines +49 to +54
Get = '2',
Post = '3',
Put = '4',
Patch = '5',
Del = '6',
Head = '7',
Copy link
Member

Choose a reason for hiding this comment

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

Non-blocker, but I should have asked on previous PR: Is there a ref link we can include from which these numbers originate? (And against which they can be validated?) Or, is this the canonical source?

Copy link
Contributor

Choose a reason for hiding this comment

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

This will be an after-release task, we are setting the canon, but there will be a source of truth external to this. We are just the first library on this.

Comment on lines -112 to +117
let libraryHeaders = {};

if (Platform.isReactNative) {
const userAgent = Platform.userAgent || 'aws-amplify/0.1.x';
libraryHeaders = {
'User-Agent': userAgent,
};
}
const libraryHeaders = {
[USER_AGENT_HEADER]: getAmplifyUserAgentString(
init.customUserAgentDetails
),
};
Copy link
Member

Choose a reason for hiding this comment

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

Like. Subscribe.

return this._restApi.head(
apiName,
path,
this.getInitWithCustomUserAgentDetails(init, ApiAction.Head)
Copy link
Member

Choose a reason for hiding this comment

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

Somewhat of a verbose function. Would it be simpler and more readable to separate the object merging from the user agent construction?

{...init, ...this.userAgentFor(ApiAction.Head))

Or

this.mergeInit(init, this.userAgentFor(ApiAction.Head))

Or similar?

Not even remotely a blocker. Just pondering "aloud" on this one.

@erinleigh90 erinleigh90 merged commit 4315ba0 into aws-amplify:feat/user-agent-enhancements/main Jun 6, 2023
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.

None yet

4 participants