Skip to content

Commit

Permalink
Modify behavior of Endpoint#withAttrs to merge attributes (#5802)
Browse files Browse the repository at this point in the history
Motivation:

It seems inconsistent that `Endpoint#withAttr` merges the attribute to
the existing attributes, whereas `Endpoint#withAttrs` simply replaces
the previous attributes.
I propose that we modify the behavior of `Endpoint#withAttrs` to be
aligned with `Endpoint#withAttr`. Additionally, in order to support the
previous behavior, I propose a new API `Endpoint#replaceAttrs` be added.

I've confirmed that there are no uses of this API at least internally.

ref: #5785 (comment)

Modifications:

- Modified the behavior of `Endpoint#withAttrs` to merge attributes
- Added a new API `Endpoint#replaceAttrs` to support the previous
behavior

Result:

- More consistent APIs
- Preparation for #5785

<!--
Visit this URL to learn more about how to write a pull request
description:

https://armeria.dev/community/developer-guide#how-to-write-pull-request-description
-->
  • Loading branch information
jrhee17 authored Jul 25, 2024
1 parent 450d493 commit d793ade
Show file tree
Hide file tree
Showing 2 changed files with 61 additions and 7 deletions.
30 changes: 27 additions & 3 deletions core/src/main/java/com/linecorp/armeria/client/Endpoint.java
Original file line number Diff line number Diff line change
Expand Up @@ -696,25 +696,49 @@ public <T> Endpoint withAttr(AttributeKey<T> key, @Nullable T value) {
if (value == null) {
return this;
}
return withAttrs(Attributes.of(key, value));
return replaceAttrs(Attributes.of(key, value));
}

if (attributes.attr(key) == value) {
return this;
} else {
final AttributesBuilder attributesBuilder = attributes.toBuilder();
attributesBuilder.set(key, value);
return withAttrs(attributesBuilder.build());
return replaceAttrs(attributesBuilder.build());
}
}

/**
* Returns a new {@link Endpoint} with the specified {@link Attributes}.
* Note that the {@link #attrs()} of this {@link Endpoint} is merged with the specified
* {@link Attributes}. For attributes with the same {@link AttributeKey}, the attribute
* in {@param newAttributes} has higher precedence.
*/
@UnstableApi
@SuppressWarnings("unchecked")
public Endpoint withAttrs(Attributes newAttributes) {
requireNonNull(newAttributes, "newAttributes");
if (newAttributes.isEmpty()) {
return this;
}
if (attrs().isEmpty()) {
return replaceAttrs(newAttributes);
}
final AttributesBuilder builder = attrs().toBuilder();
newAttributes.attrs().forEachRemaining(entry -> {
final AttributeKey<Object> key = (AttributeKey<Object>) entry.getKey();
builder.set(key, entry.getValue());
});
return new Endpoint(type, host, ipAddr, port, weight, builder.build());
}

/**
* Returns a new {@link Endpoint} with the specified {@link Attributes}.
* Note that the {@link #attrs()} of this {@link Endpoint} is replaced with the specified
* {@link Attributes}.
*/
@UnstableApi
public Endpoint withAttrs(Attributes newAttributes) {
public Endpoint replaceAttrs(Attributes newAttributes) {
requireNonNull(newAttributes, "newAttributes");
if (attrs().isEmpty() && newAttributes.isEmpty()) {
return this;
Expand Down
38 changes: 34 additions & 4 deletions core/src/test/java/com/linecorp/armeria/client/EndpointTest.java
Original file line number Diff line number Diff line change
Expand Up @@ -745,8 +745,8 @@ void attrs() {
attrs2.set(key1, "value1-2");
attrs2.set(key3, "value3");

final Endpoint endpointB = endpoint.withAttrs(attrs.build());
final Endpoint endpointC = endpointB.withAttrs(attrs2.build());
final Endpoint endpointB = endpoint.replaceAttrs(attrs.build());
final Endpoint endpointC = endpointB.replaceAttrs(attrs2.build());

assertThat(endpointB.attr(key1))
.isEqualTo("value1");
Expand All @@ -767,10 +767,40 @@ void attrs() {
Maps.immutableEntry(key3, "value3"));

// Reset attrs with an empty attributes.
final Endpoint newEndpointB = endpointB.withAttrs(Attributes.of());
final Endpoint newEndpointB = endpointB.replaceAttrs(Attributes.of());
assertThat(newEndpointB.attrs().isEmpty()).isTrue();

final Endpoint sameEndpoint = endpoint.withAttrs(Attributes.of());
final Endpoint sameEndpoint = endpoint.replaceAttrs(Attributes.of());
assertThat(sameEndpoint).isSameAs(endpoint);
}

@Test
void withAttrs() {
final AttributeKey<String> key1 = AttributeKey.valueOf("key1");
final AttributeKey<String> key2 = AttributeKey.valueOf("key2");

final Endpoint endpoint = Endpoint.parse("a").withAttrs(Attributes.of(key1, "val1"));
assertThat(endpoint.attrs().attrs())
.toIterable()
.containsExactlyInAnyOrder(Maps.immutableEntry(key1, "val1"));

assertThat(endpoint.withAttrs(Attributes.of(key2, "val2")).attrs().attrs())
.toIterable()
.containsExactlyInAnyOrder(Maps.immutableEntry(key1, "val1"),
Maps.immutableEntry(key2, "val2"));

assertThat(endpoint.withAttrs(Attributes.of(key1, "val1")).attrs().attrs())
.toIterable()
.containsExactlyInAnyOrder(Maps.immutableEntry(key1, "val1"));

assertThat(endpoint.withAttrs(Attributes.of(key1, "val2")).attrs().attrs())
.toIterable()
.containsExactlyInAnyOrder(Maps.immutableEntry(key1, "val2"));

assertThat(endpoint.withAttrs(Attributes.of()).attrs().attrs())
.toIterable()
.containsExactlyInAnyOrder(Maps.immutableEntry(key1, "val1"));

assertThat(endpoint.withAttrs(Attributes.of())).isSameAs(endpoint);
}
}

0 comments on commit d793ade

Please sign in to comment.