Add new field "description" in attribute_metadata.proto #233
Conversation
…ges to accommodate in AttributeMetadataModelTest
…ges to accommodate in AttributeMetadataModelTest
…nto description-field-in-metadata # Conflicts: # attribute-service-impl/src/test/java/org/hypertrace/core/attribute/service/model/AttributeMetadataModelTest.java
| private String displayName; | ||
|
|
||
| @JsonProperty(value = "description") | ||
| private String description; |
There was a problem hiding this comment.
The existing documents will not have this property. We need to check if it defaults to null in that case.
There was a problem hiding this comment.
This should be fine as ObjectMapper().configure(DeserializationFeature.FAIL_ON_UNKNOWN_PROPERTIES, false), but lets see if we have a test for it.
...e-impl/src/main/java/org/hypertrace/core/attribute/service/model/AttributeMetadataModel.java
Outdated
Show resolved
Hide resolved
...e-impl/src/main/java/org/hypertrace/core/attribute/service/model/AttributeMetadataModel.java
Outdated
Show resolved
Hide resolved
...e-impl/src/main/java/org/hypertrace/core/attribute/service/model/AttributeMetadataModel.java
Outdated
Show resolved
Hide resolved
...e-impl/src/main/java/org/hypertrace/core/attribute/service/model/AttributeMetadataModel.java
Outdated
Show resolved
Hide resolved
| .append("\",\"tenant_id\":\"") | ||
| .append(tenantId) | ||
| .append("\",\"description\":\"") | ||
| .append(scope.name()) |
There was a problem hiding this comment.
We are copying the scope.name() as the description.
Can we keep the existing tests as they are and add an additional test for the description? Since the description string is optional, the existing tests should work fine, right?
There was a problem hiding this comment.
You are right, I've undone any changes I made to this file. Do we need separate tests for description apart from the ones added in AttributeMetadataModelTest?
| + "\"id\":\"EVENT.key\"," | ||
| + "\"value_kind\":\"TYPE_STRING\"," | ||
| + "\"display_name\":\"Display\"," | ||
| + "\"description\":\"\"," |
There was a problem hiding this comment.
If it's without definitionAndDescription, we don't need an empty "" string, right? Are we missing something?
There was a problem hiding this comment.
If it's without definitionAndDescription, we don't need an empty "" string, right? Are we missing something?
I think this is because the default string value is an empty string in protobuf, and this test has two parts - one from fromJson, and the second from fromDTO. Got it.
There was a problem hiding this comment.
Can you add an explicit test testDescriptionBackwardsCompatibility?
There was a problem hiding this comment.
I've reverted the testAttributeDefinitionBackwardsCompatibility to it's original state and added new tests in testAttributeDescriptionBackwardsCompatibility
...service-api/src/main/proto/org/hypertrace/core/attribute/service/v1/attribute_metadata.proto
Show resolved
Hide resolved
| private boolean internal; | ||
|
|
||
| @JsonProperty(value = "description") | ||
| private String description = ""; |
There was a problem hiding this comment.
| private String description = ""; | |
| private String description; |
Adding support for description for an attribute in the AttributeMetadata class. The description is expected to be used for documentation down the line.
Default value for the new field is an empty string.