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.

Sign up for GitHub

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

Jump to bottom

add a structured type to batchGet in OpenAPI V3 spec #10956

Merged
merged 6 commits into from
Jul 22, 2024
Merged

add a structured type to batchGet in OpenAPI V3 spec #10956

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
84375d2
add a structured type to batchGet in OpenAPI V3 spec
kevin1chun Jul 19, 2024
e184bcd
fix bug with aspect POST request body - expected request body is the …
kevin1chun Jul 20, 2024
73fb5e9
update default
kevin1chun Jul 22, 2024
143554d
lint
kevin1chun Jul 22, 2024
a26f3bb
add @Hidden to V3 Entity controller as all endpoints are registered v…
kevin1chun Jul 22, 2024
e377b43
extract the aspect json from the of the request body. Revert change …
kevin1chun Jul 22, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 8 additions & 5 deletions ...servlet/src/main/java/io/datahubproject/openapi/controller/GenericEntitiesController.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@
import com.datahub.authorization.AuthorizerChain;
import com.datahub.util.RecordUtils;
import com.fasterxml.jackson.core.JsonProcessingException;
import com.fasterxml.jackson.databind.JsonNode;
import com.fasterxml.jackson.databind.ObjectMapper;
import com.google.common.collect.ImmutableSet;
import com.linkedin.common.urn.Urn;
Expand Down Expand Up @@ -508,10 +509,10 @@ public ResponseEntity<E> createAspect(
@PathVariable("aspectName") String aspectName,
@RequestParam(value = "systemMetadata", required = false, defaultValue = "false")
Boolean withSystemMetadata,
@RequestParam(value = "createIfNotExists", required = false, defaultValue = "false")
@RequestParam(value = "createIfNotExists", required = false, defaultValue = "true")
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please note this default behavior change in updating-datahub.md as this may change workflows built around this endpoint, it does make more sense to me as a default though.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This was recently added in #10654 for us specifically and no one is using this afaik, but i will note it just in case

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actually @RyanHolstien is that necessary if there was no release published between the changes?
This change (#10654) was implemented after v0.13.3 was released

Copy link
Collaborator

Choose a reason for hiding this comment

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

Nope! That's fine if it hasn't been released yet :)

Boolean createIfNotExists,
@RequestBody @Nonnull String jsonAspect)
throws URISyntaxException {
throws URISyntaxException, JsonProcessingException {

Urn urn = validatedUrn(entityUrn);
EntitySpec entitySpec = entityRegistry.getEntitySpec(entityName);
Expand Down Expand Up @@ -649,8 +650,8 @@ protected Boolean exists(
* fixes)
*
* @param requestedAspectNames requested aspects
* @return updated map
* @param <T> map values
* @return updated map
*/
protected <T> LinkedHashMap<Urn, Map<String, T>> resolveAspectNames(
LinkedHashMap<Urn, Map<String, T>> requestedAspectNames, @Nonnull T defaultValue) {
Expand Down Expand Up @@ -732,15 +733,17 @@ protected ChangeMCP toUpsertItem(
Boolean createIfNotExists,
String jsonAspect,
Actor actor)
throws URISyntaxException {
throws JsonProcessingException {
JsonNode jsonNode = objectMapper.readTree(jsonAspect);
String aspectJson = jsonNode.get("value").toString();
return ChangeItemImpl.builder()
.urn(entityUrn)
.aspectName(aspectSpec.getName())
.changeType(Boolean.TRUE.equals(createIfNotExists) ? ChangeType.CREATE : ChangeType.UPSERT)
.auditStamp(AuditStampUtils.createAuditStamp(actor.toUrnStr()))
.recordTemplate(
GenericRecordUtils.deserializeAspect(
ByteString.copyString(jsonAspect, StandardCharsets.UTF_8),
ByteString.copyString(aspectJson, StandardCharsets.UTF_8),
GenericRecordUtils.JSON,
aspectSpec))
.build(aspectRetriever);
Expand Down
31 changes: 18 additions & 13 deletions ...ervice/openapi-servlet/src/main/java/io/datahubproject/openapi/v3/OpenAPIV3Generator.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -82,6 +82,20 @@ public static OpenAPI generateOpenApiSpec(EntityRegistry entityRegistry) {
"SystemMetadata", new Schema().type(TYPE_OBJECT).additionalProperties(true));
components.addSchemas("SortOrder", new Schema()._enum(List.of("ASCENDING", "DESCENDING")));
components.addSchemas("AspectPatch", buildAspectPatchSchema());
components.addSchemas(
"BatchGetRequestBody",
new Schema<>()
.type(TYPE_OBJECT)
.description("Request body for batch get aspects.")
.properties(
Map.of(
"headers",
new Schema<>()
.type(TYPE_OBJECT)
.additionalProperties(new Schema<>().type(TYPE_STRING))
.description("System headers for the operation.")
.nullable(true)))
.nullable(true));
entityRegistry
.getAspectSpecs()
.values()
Expand Down Expand Up @@ -645,28 +659,19 @@ private static Schema buildEntityScrollSchema(final EntitySpec entity) {
private static Schema buildEntityBatchGetRequestSchema(
final EntitySpec entity, Set<String> aspectNames) {

final Schema stringTypeSchema = new Schema<>();
stringTypeSchema.setType(TYPE_STRING);
final Map<String, Schema> headers =
Map.of(
"headers",
new Schema<>()
.type(TYPE_OBJECT)
.additionalProperties(stringTypeSchema)
.description("System headers for the operation.")
.nullable(true));

final Map<String, Schema> properties =
entity.getAspectSpecMap().entrySet().stream()
.filter(a -> aspectNames.contains(a.getValue().getName()))
.collect(
Collectors.toMap(
Map.Entry::getKey, a -> new Schema().type(TYPE_OBJECT).properties(headers)));
Map.Entry::getKey,
a -> new Schema().$ref("#/components/schemas/BatchGetRequestBody")));
properties.put(
PROPERTY_URN,
new Schema<>().type(TYPE_STRING).description("Unique id for " + entity.getName()));

properties.put(entity.getKeyAspectName(), new Schema().type(TYPE_OBJECT).properties(headers));
properties.put(
entity.getKeyAspectName(), new Schema().$ref("#/components/schemas/BatchGetRequestBody"));

return new Schema<>()
.type(TYPE_OBJECT)
Expand Down
2 changes: 2 additions & 0 deletions ...enapi-servlet/src/main/java/io/datahubproject/openapi/v3/controller/EntityController.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,7 @@
import io.datahubproject.openapi.v3.models.GenericAspectV3;
import io.datahubproject.openapi.v3.models.GenericEntityScrollResultV3;
import io.datahubproject.openapi.v3.models.GenericEntityV3;
import io.swagger.v3.oas.annotations.Hidden;
import io.swagger.v3.oas.annotations.Operation;
import io.swagger.v3.oas.annotations.tags.Tag;
import jakarta.servlet.http.HttpServletRequest;
Expand Down Expand Up @@ -67,6 +68,7 @@
@RequiredArgsConstructor
@RequestMapping("/v3/entity")
@Slf4j
@Hidden
public class EntityController
extends GenericEntitiesController<
GenericAspectV3, GenericEntityV3, GenericEntityScrollResultV3> {
Expand Down
13 changes: 13 additions & 0 deletions ...ce/openapi-servlet/src/test/java/io/datahubproject/openapi/v3/OpenAPIV3GeneratorTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -88,5 +88,18 @@ public void testOpenApiSpecBuilder() throws Exception {
Schema fabricType = openAPI.getComponents().getSchemas().get("FabricType");
assertEquals("string", fabricType.getType());
assertFalse(fabricType.getEnum().isEmpty());

Map<String, Schema> batchProperties =
openAPI
.getComponents()
.getSchemas()
.get("BatchGetContainerEntityRequest_v3")
.getProperties();
batchProperties.entrySet().stream()
.filter(entry -> !entry.getKey().equals("urn"))
Copy link
Collaborator

Choose a reason for hiding this comment

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

Probably makes more sense to do this as a anyMatch() rather than filtering specific entries, no? Otherwise this test will likely need to be updated every time a new component gets added.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So I specifically wanted to test all attributes in this batchGet request body except urn.
Reasoning is that all attributes should be mapped to a specific format, but urn is a standalone string that is used as a key

.forEach(
entry ->
assertEquals(
"#/components/schemas/BatchGetRequestBody", entry.getValue().get$ref()));
}
}
Loading