fix: handle clientContextParam collisions with builtin config keys #1788
Conversation
smilkuri
force-pushed
the
resolve-conflict-params
branch
from
4a0b1ea to
c0cc0c7
Compare
smilkuri
force-pushed
the
resolve-conflict-params
branch
from
f3ea1fe to
13fd0f0
Compare
smilkuri
force-pushed
the
resolve-conflict-params
branch
from
cfaa5d4 to
9117008
Compare
smilkuri
force-pushed
the
resolve-conflict-params
branch
from
9117008 to
1fc37f8
Compare
smilkuri
changed the title
| const config = clientConfig as typeof clientConfig & { clientContextParams?: Record<string, unknown> }; | ||
| if (config.clientContextParams === undefined) { | ||
| config.clientContextParams = {}; | ||
| } |
There was a problem hiding this comment.
no need to do this, since the only place where it's read also accounts for it being undefined
| const clientContextParams = config.clientContextParams as Record<string, unknown> | undefined; | ||
| const nestedValue: unknown = clientContextParams?.[configKey] ?? clientContextParams?.[canonicalEndpointParamKey]; | ||
| // Fall back to direct config properties | ||
| const configValue: unknown = nestedValue ?? config[configKey] ?? config[canonicalEndpointParamKey]; |
There was a problem hiding this comment.
This should only happen when the configuration being read is a client context parameter.
Users should not be able to place other kinds of config into this object and have them be read. It will create a contract where none is intended.
| return Object.assign(options, { | ||
| stage: options.stage ?? "production", | ||
| defaultSigningName: "", | ||
| clientContextParams: Object.assign(clientContextParamDefaults, options.clientContextParams ?? {}), |
There was a problem hiding this comment.
no need to default the input to ?? {}, since object assign will ignore undefined values.
smilkuri
force-pushed
the
resolve-conflict-params
branch
from
ef3cda8 to
c5f0d86
Compare
| Set<String> knownConfigKeys = Set.of( | ||
| "apiKey", "retryStrategy", "requestHandler"); | ||
| // Generate clientContextParams with all params excluding built-ins | ||
| Map<String, String> customerContextParams = new HashMap<>(); |
There was a problem hiding this comment.
call this customContextParams, not customer
| writer.addTypeImport("EndpointParameters", "__EndpointParameters", TypeScriptDependency.SMITHY_TYPES); | ||
| writer.addTypeImport("Provider", null, TypeScriptDependency.SMITHY_TYPES); | ||
| writer.addImport("EndpointParameters", "__EndpointParameters", TypeScriptDependency.SMITHY_TYPES); | ||
| writer.addImport("Provider", null, TypeScriptDependency.SMITHY_TYPES); |
There was a problem hiding this comment.
no reason to change these from type import to import
| Map<String, String> builtInParams = ruleSetParameterFinder.getBuiltInParams(); | ||
| builtInParams.keySet().removeIf(OmitEndpointParams::isOmitted); | ||
| Set<String> knownConfigKeys = Set.of( | ||
| "apiKey", "retryStrategy", "requestHandler"); |
There was a problem hiding this comment.
make this static final on the class
There was a problem hiding this comment.
on second thought this should be handled by a separate class.
seed the class with the config keys seen in https://github.com/aws/aws-sdk-js-v3/blob/f806a2b9eded9f82d488f3d81db36b9cba9da220/private/aws-client-api-test/src/client-interface-tests/client-s3/impl/initializeWithMaximalConfiguration.ts except those specific to S3, then create a public Java static method to add keys to the list.
2 participants
Issue #, if available:
#1779
codegen v3: aws/aws-sdk-js-v3#7527
Description of changes:
--