Skip to content

Commit

Permalink
Refactor GetApiKeyResponse and QueryApiKeyResponse to return List ins…
Browse files Browse the repository at this point in the history 
…tead of arrays (#106409)

The plan is that GetApiKeyResponse and QueryApiKeyResponse
will be receiving collections of api key info, optional profile uids, and optional
sort values, that they will "zip" into a single list of records.
Array vs List is not important in this context, but we use collections in the
internal services and action handlers, so it is more consistent to also use them
in the responses.
  • Loading branch information
@albertzaharovits
albertzaharovits committed Mar 19, 2024
1 parent 43b8ca0 commit 963a5b8
Show file tree
Hide file tree
Showing 11 changed files with 223 additions and 204 deletions.
68 changes: 48 additions & 20 deletions .../src/main/java/org/elasticsearch/xpack/core/security/action/apikey/GetApiKeyResponse.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -29,47 +29,75 @@
*/
public final class GetApiKeyResponse extends ActionResponse implements ToXContentObject {

private final ApiKey[] foundApiKeysInfo;
public static final GetApiKeyResponse EMPTY = new GetApiKeyResponse(List.of());

public GetApiKeyResponse(Collection<ApiKey> foundApiKeysInfo) {
Objects.requireNonNull(foundApiKeysInfo, "found_api_keys_info must be provided");
this.foundApiKeysInfo = foundApiKeysInfo.toArray(new ApiKey[0]);
}
private final List<Item> foundApiKeyInfoList;

public static GetApiKeyResponse emptyResponse() {
return new GetApiKeyResponse(List.of());
public GetApiKeyResponse(Collection<ApiKey> foundApiKeyInfos) {
Objects.requireNonNull(foundApiKeyInfos, "found_api_keys_info must be provided");
this.foundApiKeyInfoList = foundApiKeyInfos.stream().map(Item::new).toList();
}

public ApiKey[] getApiKeyInfos() {
return foundApiKeysInfo;
public List<Item> getApiKeyInfoList() {
return foundApiKeyInfoList;
}

@Override
public XContentBuilder toXContent(XContentBuilder builder, Params params) throws IOException {
builder.startObject().array("api_keys", (Object[]) foundApiKeysInfo);
return builder.endObject();
builder.startObject();
builder.field("api_keys", foundApiKeyInfoList);
builder.endObject();
return builder;
}

@Override
public void writeTo(StreamOutput out) throws IOException {
TransportAction.localOnly();
}

@SuppressWarnings("unchecked")
static final ConstructingObjectParser<GetApiKeyResponse, Void> PARSER = new ConstructingObjectParser<>("get_api_key_response", args -> {
return (args[0] == null) ? GetApiKeyResponse.emptyResponse() : new GetApiKeyResponse((List<ApiKey>) args[0]);
});
static {
PARSER.declareObjectArray(optionalConstructorArg(), (p, c) -> ApiKey.fromXContent(p), new ParseField("api_keys"));
@Override
public boolean equals(Object o) {
if (this == o) return true;
if (o == null || getClass() != o.getClass()) return false;
GetApiKeyResponse that = (GetApiKeyResponse) o;
return Objects.equals(foundApiKeyInfoList, that.foundApiKeyInfoList);
}

public static GetApiKeyResponse fromXContent(XContentParser parser) throws IOException {
return PARSER.parse(parser, null);
@Override
public int hashCode() {
return Objects.hash(foundApiKeyInfoList);
}

@Override
public String toString() {
return "GetApiKeyResponse [foundApiKeysInfo=" + foundApiKeysInfo + "]";
return "GetApiKeyResponse{foundApiKeysInfo=" + foundApiKeyInfoList + "}";
}

public record Item(ApiKey apiKeyInfo) implements ToXContentObject {
@Override
public XContentBuilder toXContent(XContentBuilder builder, Params params) throws IOException {
builder.startObject();
apiKeyInfo.innerToXContent(builder, params);
builder.endObject();
return builder;
}

@Override
public String toString() {
return "Item{apiKeyInfo=" + apiKeyInfo + "}";
}
}

@SuppressWarnings("unchecked")
static final ConstructingObjectParser<GetApiKeyResponse, Void> PARSER = new ConstructingObjectParser<>(
"get_api_key_response",
args -> (args[0] == null) ? GetApiKeyResponse.EMPTY : new GetApiKeyResponse((List<ApiKey>) args[0])
);
static {
PARSER.declareObjectArray(optionalConstructorArg(), (p, c) -> ApiKey.fromXContent(p), new ParseField("api_keys"));
}

public static GetApiKeyResponse fromXContent(XContentParser parser) throws IOException {
return PARSER.parse(parser, null);
}
}
70 changes: 19 additions & 51 deletions ...rc/main/java/org/elasticsearch/xpack/core/security/action/apikey/QueryApiKeyResponse.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@
import org.elasticsearch.xcontent.XContentBuilder;

import java.io.IOException;
import java.util.ArrayList;
import java.util.Arrays;
import java.util.Collection;
import java.util.List;
Expand All @@ -27,31 +28,25 @@
*/
public final class QueryApiKeyResponse extends ActionResponse implements ToXContentObject {

public static final QueryApiKeyResponse EMPTY = new QueryApiKeyResponse(0, List.of(), null);

private final long total;
private final Item[] items;
private final List<Item> foundApiKeyInfoList;
private final @Nullable InternalAggregations aggregations;

public QueryApiKeyResponse(long total, Collection<Item> items, @Nullable InternalAggregations aggregations) {
this.total = total;
Objects.requireNonNull(items, "items must be provided");
this.items = items.toArray(new Item[0]);
this.foundApiKeyInfoList = items instanceof List<Item> ? (List<Item>) items : new ArrayList<>(items);
this.aggregations = aggregations;
}

public static QueryApiKeyResponse emptyResponse() {
return new QueryApiKeyResponse(0, List.of(), null);
}

public long getTotal() {
return total;
}

public Item[] getItems() {
return items;
}

public int getCount() {
return items.length;
public List<Item> getApiKeyInfoList() {
return foundApiKeyInfoList;
}

public InternalAggregations getAggregations() {
Expand All @@ -60,11 +55,13 @@ public InternalAggregations getAggregations() {

@Override
public XContentBuilder toXContent(XContentBuilder builder, Params params) throws IOException {
builder.startObject().field("total", total).field("count", items.length).array("api_keys", (Object[]) items);
builder.startObject();
builder.field("total", total).field("count", foundApiKeyInfoList.size()).field("api_keys", foundApiKeyInfoList);
if (aggregations != null) {
aggregations.toXContent(builder, params);
}
return builder.endObject();
builder.endObject();
return builder;
}

@Override
Expand All @@ -77,69 +74,40 @@ public boolean equals(Object o) {
if (this == o) return true;
if (o == null || getClass() != o.getClass()) return false;
QueryApiKeyResponse that = (QueryApiKeyResponse) o;
return total == that.total && Arrays.equals(items, that.items) && Objects.equals(aggregations, that.aggregations);
return total == that.total
&& Objects.equals(foundApiKeyInfoList, that.foundApiKeyInfoList)
&& Objects.equals(aggregations, that.aggregations);
}

@Override
public int hashCode() {
int result = Objects.hash(total);
result = 31 * result + Arrays.hashCode(items);
result = 31 * result + Objects.hash(foundApiKeyInfoList);
result = 31 * result + Objects.hash(aggregations);
return result;
}

@Override
public String toString() {
return "QueryApiKeyResponse{total=" + total + ", items=" + Arrays.toString(items) + ", aggs=" + aggregations + "}";
return "QueryApiKeyResponse{total=" + total + ", items=" + foundApiKeyInfoList + ", aggs=" + aggregations + "}";
}

public static class Item implements ToXContentObject {
private final ApiKey apiKey;
@Nullable
private final Object[] sortValues;

public Item(ApiKey apiKey, @Nullable Object[] sortValues) {
this.apiKey = apiKey;
this.sortValues = sortValues;
}

public ApiKey getApiKey() {
return apiKey;
}

public Object[] getSortValues() {
return sortValues;
}
public record Item(ApiKey apiKeyInfo, @Nullable Object[] sortValues) implements ToXContentObject {

@Override
public XContentBuilder toXContent(XContentBuilder builder, Params params) throws IOException {
builder.startObject();
apiKey.innerToXContent(builder, params);
apiKeyInfo.innerToXContent(builder, params);
if (sortValues != null && sortValues.length > 0) {
builder.array("_sort", sortValues);
}
builder.endObject();
return builder;
}

@Override
public boolean equals(Object o) {
if (this == o) return true;
if (o == null || getClass() != o.getClass()) return false;
Item item = (Item) o;
return Objects.equals(apiKey, item.apiKey) && Arrays.equals(sortValues, item.sortValues);
}

@Override
public int hashCode() {
int result = Objects.hash(apiKey);
result = 31 * result + Arrays.hashCode(sortValues);
return result;
}

@Override
public String toString() {
return "Item{" + "apiKey=" + apiKey + ", sortValues=" + Arrays.toString(sortValues) + '}';
return "Item{apiKeyInfo=" + apiKeyInfo + ", sortValues=" + Arrays.toString(sortValues) + '}';
}
}
}
7 changes: 5 additions & 2 deletions ...ity-trial/src/javaRestTest/java/org/elasticsearch/xpack/security/apikey/ApiKeyRestIT.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1824,9 +1824,12 @@ private void expectMetadata(final String apiKeyId, final Map<String, Object> exp
assertOK(response);
try (XContentParser parser = responseAsParser(response)) {
final var apiKeyResponse = GetApiKeyResponse.fromXContent(parser);
assertThat(apiKeyResponse.getApiKeyInfos().length, equalTo(1));
assertThat(apiKeyResponse.getApiKeyInfoList().size(), equalTo(1));
// ApiKey metadata is set to empty Map if null
assertThat(apiKeyResponse.getApiKeyInfos()[0].getMetadata(), equalTo(expectedMetadata == null ? Map.of() : expectedMetadata));
assertThat(
apiKeyResponse.getApiKeyInfoList().get(0).apiKeyInfo().getMetadata(),
equalTo(expectedMetadata == null ? Map.of() : expectedMetadata)
);
}
}

Expand Down
36 changes: 19 additions & 17 deletions ...trial/src/javaRestTest/java/org/elasticsearch/xpack/security/apikey/GetApiKeysRestIT.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -27,18 +27,17 @@
import org.junit.Before;

import java.io.IOException;
import java.util.Arrays;
import java.util.HashMap;
import java.util.List;
import java.util.Map;
import java.util.Set;
import java.util.stream.Collectors;

import javax.annotation.Nullable;

import static org.hamcrest.Matchers.containsInAnyOrder;
import static org.hamcrest.Matchers.emptyArray;
import static org.hamcrest.Matchers.emptyIterable;
import static org.hamcrest.Matchers.equalTo;
import static org.hamcrest.Matchers.iterableWithSize;
import static org.hamcrest.Matchers.notNullValue;
import static org.hamcrest.Matchers.nullValue;

Expand Down Expand Up @@ -103,7 +102,7 @@ public void testGetApiKeysWithActiveOnlyFlag() throws Exception {

// We get an empty result when no API keys active
getSecurityClient().invalidateApiKeys(apiKeyId1);
assertThat(getApiKeysWithRequestParams(Map.of("active_only", "true")).getApiKeyInfos(), emptyArray());
assertThat(getApiKeysWithRequestParams(Map.of("active_only", "true")).getApiKeyInfoList(), emptyIterable());

{
// Using together with id parameter, returns 404 for inactive key
Expand Down Expand Up @@ -166,12 +165,12 @@ public void testGetApiKeysWithActiveOnlyFlagAndMultipleUsers() throws Exception
manageApiKeyUserApiKeyId
);
assertThat(
getApiKeysWithRequestParams(Map.of("active_only", "true", "username", MANAGE_OWN_API_KEY_USER)).getApiKeyInfos(),
emptyArray()
getApiKeysWithRequestParams(Map.of("active_only", "true", "username", MANAGE_OWN_API_KEY_USER)).getApiKeyInfoList(),
emptyIterable()
);
assertThat(
getApiKeysWithRequestParams(MANAGE_OWN_API_KEY_USER, Map.of("active_only", "true", "owner", "true")).getApiKeyInfos(),
emptyArray()
getApiKeysWithRequestParams(MANAGE_OWN_API_KEY_USER, Map.of("active_only", "true", "owner", "true")).getApiKeyInfoList(),
emptyIterable()
);

// No more active API keys
Expand All @@ -180,15 +179,15 @@ public void testGetApiKeysWithActiveOnlyFlagAndMultipleUsers() throws Exception
assertThat(
getApiKeysWithRequestParams(
Map.of("active_only", "true", "username", randomFrom(MANAGE_SECURITY_USER, MANAGE_OWN_API_KEY_USER))
).getApiKeyInfos(),
emptyArray()
).getApiKeyInfoList(),
emptyIterable()
);
assertThat(
getApiKeysWithRequestParams(
randomFrom(MANAGE_SECURITY_USER, MANAGE_OWN_API_KEY_USER),
Map.of("active_only", "true", "owner", "true")
).getApiKeyInfos(),
emptyArray()
).getApiKeyInfoList(),
emptyIterable()
);
// With flag set to false, we get both inactive keys
assertResponseContainsApiKeyIds(
Expand All @@ -205,8 +204,8 @@ public void testInvalidateApiKey() throws Exception {
setUserForRequest(request, MANAGE_SECURITY_USER);
GetApiKeyResponse getApiKeyResponse = GetApiKeyResponse.fromXContent(getParser(client().performRequest(request)));

assertThat(getApiKeyResponse.getApiKeyInfos().length, equalTo(1));
ApiKey apiKey = getApiKeyResponse.getApiKeyInfos()[0];
assertThat(getApiKeyResponse.getApiKeyInfoList(), iterableWithSize(1));
ApiKey apiKey = getApiKeyResponse.getApiKeyInfoList().get(0).apiKeyInfo();
assertThat(apiKey.isInvalidated(), equalTo(false));
assertThat(apiKey.getInvalidation(), nullValue());
assertThat(apiKey.getId(), equalTo(apiKeyId0));
Expand All @@ -226,8 +225,8 @@ public void testInvalidateApiKey() throws Exception {
setUserForRequest(request, MANAGE_SECURITY_USER);
getApiKeyResponse = GetApiKeyResponse.fromXContent(getParser(client().performRequest(request)));

assertThat(getApiKeyResponse.getApiKeyInfos().length, equalTo(1));
apiKey = getApiKeyResponse.getApiKeyInfos()[0];
assertThat(getApiKeyResponse.getApiKeyInfoList(), iterableWithSize(1));
apiKey = getApiKeyResponse.getApiKeyInfoList().get(0).apiKeyInfo();
assertThat(apiKey.isInvalidated(), equalTo(true));
assertThat(apiKey.getInvalidation(), notNullValue());
assertThat(apiKey.getId(), equalTo(apiKeyId0));
Expand All @@ -245,7 +244,10 @@ private GetApiKeyResponse getApiKeysWithRequestParams(String userOnRequest, Map<
}

private static void assertResponseContainsApiKeyIds(GetApiKeyResponse response, String... ids) {
assertThat(Arrays.stream(response.getApiKeyInfos()).map(ApiKey::getId).collect(Collectors.toList()), containsInAnyOrder(ids));
assertThat(
response.getApiKeyInfoList().stream().map(GetApiKeyResponse.Item::apiKeyInfo).map(ApiKey::getId).toList(),
containsInAnyOrder(ids)
);
}

private static XContentParser getParser(Response response) throws IOException {
Expand Down

0 comments on commit 963a5b8

Please sign in to comment.