Saikred/feature/output handling #3293
Conversation
| * The class is used for Response Handling and Parsing the metadata fetched by the get call in the {@link Ec2Metadata} interface. | ||
| * Metadata is stored in the instance variable <b>body</b>. The class provides convenience methods to the users to parse the | ||
| * metadata as a String, List and to parse the metadata in the JsonResponse according to the key. | ||
| */ |
There was a problem hiding this comment.
We can exclude information that is internal to the class that might change, like that data is stored in the body field. That information isn't particularly relevant to a user of the class and it might change without someone updating the docs. It could be included as a comment in the field, if it's useful.
| if (null != body && body.contains("\n")) { | ||
|
|
||
| return Arrays.asList(body.split("\n")); | ||
| } | ||
|
|
||
| return Collections.emptyList(); |
There was a problem hiding this comment.
If it's a single line, should we return that single line in a collection instead of not returning anything?
|
|
||
| public String[] getStringArrayValuesFromJson(String key) { | ||
|
|
||
| if (null != key) { |
There was a problem hiding this comment.
should we throw on a null key?
| */ | ||
| public List<String> asList() { | ||
|
|
||
| if (null != body && body.contains("\n")) { |
There was a problem hiding this comment.
should we just validate that body is not null when an instance is created? or is there a use-case where body needs to be null?
There was a problem hiding this comment.
Just learned about the Validate class. Will use it.
| * </pre> | ||
| */ | ||
|
|
||
| public String[] getStringArrayValuesFromJson(String key) { |
There was a problem hiding this comment.
Instead of having these json-like methods, maybe we should just have one to parse the body into a software.amazon.awssdk.core.document.Document? That would require making some currently-internal stuff protected (parsing JSON into a document), but it would be more flexible.
E.g.
Ec2MetadataClient ec2Metadata = Ec2MetadataClient.create();
MetadataResponse metadataResponse = client.get("/latest/dynamic/instance-identity/document");
Document instanceIdentity = metadataResponse.asDocument();There was a problem hiding this comment.
Great insight. Will implement it.
|
|
||
| private static final Logger log = LoggerFactory.getLogger(MetadataResponse.class); | ||
|
|
||
| private static final JsonNodeParser JSON_NODE_PARSER = JsonNode.parser(); |
There was a problem hiding this comment.
We probably want removeErrorLocations to be true on this parser, so that if credentials are in the metadata it won't include them in the exception message (which might be logged).
| MetadataResponse metadataResponse = ec2Metadata.get("/latest/meta-data/ami-id"); | ||
| assertThat(metadataResponse).isNull(); |
There was a problem hiding this comment.
Returning null seems weird here. Any reason we wouldn't throw an exception? I'd think we'd never want get() to return null.
| .filter(JsonNode::isString) | ||
| .map(JsonNode::asString) | ||
| .toArray(String[]::new); | ||
| } catch (Exception e) { |
There was a problem hiding this comment.
Any specific reason we're swallowing this exception instead of throwing it?
| <dependency> | ||
| <groupId>software.amazon.awssdk</groupId> | ||
| <artifactId>aws-json-protocol</artifactId> | ||
| <version>${awsjavasdk.version}</version> | ||
| <scope>compile</scope> | ||
| </dependency> |
There was a problem hiding this comment.
We should put something in the backlog to avoid needing this dependency at GA. I know it was needed for the DocumentMarshaller, but I think it's relatively small and we could copy it to avoid the dependency.
|
SonarCloud Quality Gate failed. |
2 participants
Created Metadata Response class to handle the response and provide convenient method.
Motivation and Context
This change is required for response handling and parsing outputs.
Modifications
Created Metadata Response class to handle the response and provide convenient method. Wrote Test cases for it.
Testing
Wrote Test cases for the changes and tested in IDE.
Screenshots (if appropriate)
Types of changes
Checklist
mvn installsucceedsscripts/new-changescript and following the instructions. Commit the new file created by the script in.changes/next-releasewith your changes.License