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

fix(openapi): fix sort criteria parameter #12090

Merged
merged 2 commits into from
Dec 11, 2024
Merged

fix(openapi): fix sort criteria parameter #12090

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
c498603
fix(openapi): fix sort criteria parameter
RyanHolstien Dec 10, 2024
ab9b97b
add doc
RyanHolstien Dec 10, 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
1 change: 1 addition & 0 deletions docs/how/updating-datahub.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,7 @@ This file documents any backwards-incompatible changes in DataHub and assists pe
- #12056: The DataHub Airflow plugin no longer supports Airflow 2.1 and Airflow 2.2.
- #12056: The DataHub Airflow plugin now defaults to the v2 plugin implementation.
- OpenAPI Update: PIT Keep Alive parameter added to scroll. NOTE: This parameter requires the `pointInTimeCreationEnabled` feature flag to be enabled and the `elasticSearch.implementation` configuration to be `elasticsearch`. This feature is not supported for OpenSearch at this time and the parameter will not be respected without both of these set.
- OpenAPI Update 2: Previously there was an incorrectly marked parameter named `sort` on the generic list entities endpoint for v3. This parameter is deprecated and only supports a single string value while the documentation indicates it supports a list of strings. This documentation error has been fixed and the correct field, `sortCriteria`, is now documented which supports a list of strings.

### Breaking Changes

Expand Down
20 changes: 14 additions & 6 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 @@ -57,6 +57,7 @@
import java.util.stream.Collectors;
import javax.annotation.Nonnull;
import javax.annotation.Nullable;
import org.apache.commons.lang.StringUtils;
import org.springframework.beans.factory.annotation.Autowired;
import org.springframework.beans.factory.annotation.Qualifier;
import org.springframework.http.MediaType;
Expand Down Expand Up @@ -190,7 +191,8 @@ public ResponseEntity<S> getEntities(
@RequestParam(value = "count", defaultValue = "10") Integer count,
@RequestParam(value = "query", defaultValue = "*") String query,
@RequestParam(value = "scrollId", required = false) String scrollId,
@RequestParam(value = "sort", required = false, defaultValue = "urn") String sortField,
@RequestParam(value = "sort", required = false, defaultValue = "urn") @Deprecated
String sortField,
@RequestParam(value = "sortCriteria", required = false) List<String> sortFields,
@RequestParam(value = "sortOrder", required = false, defaultValue = "ASCENDING")
String sortOrder,
Expand Down Expand Up @@ -222,14 +224,20 @@ public ResponseEntity<S> getEntities(
authentication.getActor().toUrnStr() + " is unauthorized to " + READ + " entities.");
}

SortOrder finalSortOrder =
SortOrder.valueOf(Optional.ofNullable(sortOrder).orElse("ASCENDING"));

List<SortCriterion> sortCriteria;
if (!CollectionUtils.isEmpty(sortFields)) {
if (!CollectionUtils.isEmpty(sortFields)
&& sortFields.stream().anyMatch(StringUtils::isNotBlank)) {
sortCriteria = new ArrayList<>();
sortFields.forEach(
field -> sortCriteria.add(SearchUtil.sortBy(field, SortOrder.valueOf(sortOrder))));
sortFields.stream()
.filter(StringUtils::isNotBlank)
.forEach(field -> sortCriteria.add(SearchUtil.sortBy(field, finalSortOrder)));
} else if (StringUtils.isNotBlank(sortField)) {
sortCriteria = Collections.singletonList(SearchUtil.sortBy(sortField, finalSortOrder));
} else {
sortCriteria =
Collections.singletonList(SearchUtil.sortBy(sortField, SortOrder.valueOf(sortOrder)));
sortCriteria = Collections.singletonList(SearchUtil.sortBy("urn", finalSortOrder));
}

ScrollResult result =
Expand Down
8 changes: 2 additions & 6 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 @@ -570,19 +570,15 @@ private static void addExtraParameters(final Components components) {
"SortBy" + MODEL_VERSION,
new Parameter()
.in(NAME_QUERY)
.name("sort")
.name("sortCriteria")
Copy link
Collaborator

Choose a reason for hiding this comment

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

need a note in the release notes section for this

Copy link
Collaborator

Choose a reason for hiding this comment

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

breaking change

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yes, the API controller still supports "sort", however client sdks using the spec will break the programmatic apis.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It technically doesn't remove the deprecated parameter so if you were previously sending in a single string here it would not break with these changes. It only affects the documentation and OpenAPI spec. Will add a note nonetheless.

.explode(true)
.description("Sort fields for pagination.")
.example(PROPERTY_URN)
.schema(
new Schema()
.type(TYPE_ARRAY)
._default(List.of(PROPERTY_URN))
.items(
new Schema<>()
.type(TYPE_STRING)
._enum(List.of(PROPERTY_URN))
._default(PROPERTY_URN))));
.items(new Schema<>().type(TYPE_STRING)._default(PROPERTY_URN))));
components.addParameters(
"SortOrder" + MODEL_VERSION,
new Parameter()
Expand Down
Loading