Tracking Issue for Attributes being Experimental.

[carl-mastrangelo]

In #1700 it was decided that Attributes is still experimental. This issue is acts the the reference from the source code.

[dapengzhang0]

should we un-public Attributes#keys()?

[herbyderby]

Attributes.get(Key key) current has @SuppressWarnings("unchecked"). It is actually not safe--Key.of will return a key for any and it is never enforced. (It is generally a bad practice for a method signature to use a type parameter a single time).

I suggest:

• adding Class argument to Key.of
• changing Attributes.get to return key.type.cast(data.get(key); then no suppression will be necessary

(This may be insufficient if there are any keys that use generic types)

[ejona86]

@herbyderby, is there a code snippet that would show how it breaks? Attributes.Key is the key used for setting/lookup and it uses reference equality. So there's really no way I see to break the Generics.

[herbyderby]

For example:

Key<Integer> ikey = Key.of("mykey");
Key<String> skey = (Key) ikey;

Attributes attr = Attributes.newBuilder()
    .set(skey, "foo")
    .build();

System.err.println("result: " + attr.get(ikey));

Prints "result: foo", even though get here is supposed to return an Integer.

It would be better for attr.get to fail. Currently it is delayed until you actually do something which causes a cast, if at all--methods on Object like equals and toString won't complain.

Admittedly this example requires a raw cast of the key. But given that it is not much work to make Attributes & Key actually typesafe, I think it is worth being a bit more defensive.

[ejona86]

This seems like an obvious, "you're going to have a bad time":

Key<String> skey = (Key) ikey;

That seems no different than, say:

List<Integer> ints = ...;
List<String> l = (List) ints;

[herbyderby]

List is a good example, because Collections.checkedList does exist, for reasons that also apply here:

http://docs.oracle.com/javase/7/docs/api/java/util/Collections.html#checkedCollection(java.util.Collection,%20java.lang.Class)

[ejona86]

But nobody uses checkedCollection. If you're casting, you're already going to have issues. Passing the Class is possible, but class literals are completely broken with generics. So that'd make everyone suffer for a case most people simply don't care about and won't hurt them.

[herbyderby]

I doubt there are good use cases for generic values here. But I've also realized that I'm just repeating the advice already in Effective Java 2nd ed. Item 29, so I'll give up now.

[carl-mastrangelo]

@herbyderby: I think your suggestion is just passing the buck. The problem still would exist:

Attributes attr = ...;
List<String> list = new ArrayList<>();
Key<List<String>> k1 = Key.of("yay", List.class);
Key<List<Integer>> k2 = (Key) k1;

List<Integer> uhoh = attr.get(k2);

The class cast check you add would succeed incorrectly. If you wanted a real match, you would need to pass in a TypeLiteral.

[herbyderby]

Yes there is no good solution if you want to support generic values--they have a bunch of problems. But as I mentioned, I doubt there is a real use case for that. If you really need a complex value you can always wrap it up into an AutoValue or whatever.

[ejona86]

API review:

• Key.of(debugString) is actually a bit weird; we were originally doing Key.of(name) which isn't as weird. Across all similar key types (Context, Attributes, and this one), we want to swap to the form Key.create(debugString) and Key.createWithDefault(debugString, T default). So that would mean adding create here and deprecating of().
• Consider removing keys(); need to investigate what it is being used for. We're fine with it, but it is a bit weird; we're not sure what it's useful for. We are okay with keys() here because Attributes was intended to be an immutable (and type-safe) map and is only really passed between the producer and consumer. Unlike Context and CallOptions, there doesn't seem much point to visibility-restricting certain keys (so you can't access the value if you can't get at the instance), which is the main thing having keys() would defeat. But because each key is semantically quite distinct, it is hard to determine what someone is using it for.
• Replace static newBuilder(Attributes) with non-static toBuilder(). Deprecate (and later remove) newBuilder(Attributes)

With those tweaks, looks good to stabilize.

[ejona86]

Given the performance of equals() (n^2), we should probably put in the javadoc that it is mainly intended for testing.

[SledgeHammer01]

API review:

• Key.of(debugString) is actually a bit weird; we were originally doing Key.of(name) which isn't as weird. Across all similar key types (Context, Attributes, and this one), we want to swap to the form Key.create(debugString) and Key.createWithDefault(debugString, T default). So that would mean adding create here and deprecating of().
• Consider removing keys(); need to investigate what it is being used for. We're fine with it, but it is a bit weird; we're not sure what it's useful for. We are okay with keys() here because Attributes was intended to be an immutable (and type-safe) map and is only really passed between the producer and consumer. Unlike Context and CallOptions, there doesn't seem much point to visibility-restricting certain keys (so you can't access the value if you can't get at the instance), which is the main thing having keys() would defeat. But because each key is semantically quite distinct, it is hard to determine what someone is using it for.
• Replace static newBuilder(Attributes) with non-static toBuilder(). Deprecate (and later remove) newBuilder(Attributes)

With those tweaks, looks good to stabilize.

I use .keys() to iterate through the map and copy the attributes into the Spring Boot context. .get(Key.create("remote-addr")) doesn't work.

[ejona86]

@SledgeHammer01, Attributes is used multiple different places. Which attributes are you copying and what are they used for in the Spring Boot context?

If this is for ServerCall.getAttributes(), then I'd sort of expect you to copy the Attributes itself instead of the individual items. If copying each individually, just explaining more or pointing to code could be helpful to understand your use-case.

[SledgeHammer01]

@SledgeHammer01, Attributes is used multiple different places. Which attributes are you copying and what are they used for in the Spring Boot context?

If this is for ServerCall.getAttributes(), then I'd sort of expect you to copy the Attributes itself instead of the individual items. If copying each individually, just explaining more or pointing to code could be helpful to understand your use-case.

@ejona86 Some background:

I'm using https://github.com/yidongnan/grpc-spring-boot-starter which is the Spring Boot Starter for gRPC. I'm also using OAuth as my authentication method (even on the gRPC endpoint).

For enterprise logistics purposes, I also need to log request/responses. I'm using Logbook for that. For the purpose of this issue, the field I'm interested in is the remote ip address since I need to log that.

In the gRPC authentication manager, I am using .keys() as follows:

    LinkedHashMap<String, Object> attributesMap = (LinkedHashMap<String, Object>) auth.getDetails();
    Attributes attributes = (Attributes) authentication.getDetails();

    // copy the remote-addr key

    for (Key<?> key : attributes.keys()) {
      attributesMap.put(key.toString(), attributes.get(key));
    }

Basically, at the end of the day, I need to copy the gRPC "remote-addr" attribute into attributesMap so Spring / Logbook can access it later for logging.

There is no way to get an attribute by name since:

get(Key.create("remote-addr"))

will always return null since keys are not value comparable.

So while I don't inherently need .keys() if another method was provided, I need some way to get the value out of attributes.

I ended up finding this issue because I was concerned .keys() was marked as deprecated and might be removed :).

[ejona86]

So while I don't inherently need .keys() if another method was provided, I need some way to get the value out of attributes.

@SledgeHammer01 Ah, okay. So this is maybe working as intended. I assume you found that key by doing System.out.println(attributes) and then iterating from there. Makes sense. We've not considered that flow much; we should probably change the debug strings of some of our attribute keys.

The API you're looking for should be Grpc.TRANSPORT_ATTR_REMOTE_ADDR. Using that Key will also cast to SocketAddress for you (although you may need an InetSocketAddress).

[SledgeHammer01]

So while I don't inherently need .keys() if another method was provided, I need some way to get the value out of attributes.

@SledgeHammer01 Ah, okay. So this is maybe working as intended. I assume you found that key by doing System.out.println(attributes) and then iterating from there. Makes sense. We've not considered that flow much; we should probably change the debug strings of some of our attribute keys.

The API you're looking for should be Grpc.TRANSPORT_ATTR_REMOTE_ADDR. Using that Key will also cast to SocketAddress for you (although you may need an InetSocketAddress).

@ejona86 I was able to switch to:

    Attributes attributes = (Attributes) authentication.getDetails();

    attributesMap.put(
        Grpc.TRANSPORT_ATTR_REMOTE_ADDR.toString(),
        attributes.get(Grpc.TRANSPORT_ATTR_REMOTE_ADDR));

And that worked. I guess that serves my purpose. Thanks!

[ejona86]

@SledgeHammer01, glad it works for you! Note that Grpc.TRANSPORT_ATTR_REMOTE_ADDR.toString() is just providing a debug string, so that could change in the future. You probably want to define your own string there (like just using the literal "remote-addr") so if we change/improve the debug string your application won't break.

[sz-liva]

Attributes.Key should override equals() and hashCode().

[ejona86]

Attributes.Key should override equals() and hashCode().

@sz-liva, why?

[sz-liva]

@ejona86 It is because Attributes.Key objects are stored in the data map and it is not possible to find a value in that map using Attributes.get(Attributes.Key.create("xxx")). If this was done on purpose then I suppose that there should be another way to get attribute by name.

As a temporary solution, I used

call.getAttributes().keys().stream()
            .filter(key -> Objects.equals(key.toString(), "remote-addr"))
            .map(key -> (InetSocketAddress) call.getAttributes().get(key))
            .findFirst()

but keys() method is marked as deprecated.

[SledgeHammer01]

@ejona86 It is because Attributes.Key objects are stored in the data map and it is not possible to find a value in that map using Attributes.get(Attributes.Key.create("xxx")). If this was done on purpose then I suppose that there should be another way to get attribute by name.

As a temporary solution, I used

call.getAttributes().keys().stream()
            .filter(key -> Objects.equals(key.toString(), "remote-addr"))
            .map(key -> (InetSocketAddress) call.getAttributes().get(key))
            .findFirst()

but keys() method is marked as deprecated.

See his response to me about this, right above. The keys are defined as static. I.e. Grpc.TRANSPORT_ATTR_REMOTE_ADDR.

[sz-liva]

@SledgeHammer01 Thank you. It was just not clear that all transport attribute keys are defined in the Grpc class.

[ejona86]

If this was done on purpose then I suppose that there should be another way to get attribute by name.

The thing is that attribute keys don't have a name. The string passed is a debug string. We could maybe add some more language to Attributes.Key to at least mention "uses reference equality" like we do for CallOptions.Key or fuller text like we have in Context.Key (although that full text requires keys() to be removed).

Attributes is generic and used in multiple different circumstances. The ones listed in Grpc may not be exhaustive. A specific transport can have its own, for example. But it does seem we could add a breadcrumb in the {Client,Server}Call.getAttributes() javadoc to point to Grpc as the source of some of the common attributes. We should change the debug string to also be the API name for the key; if you saw the debug string as io.grpc.Grpc.TRANSPORT_ATTR_REMOTE_ADDR then the way to use it becomes obvious.

[dingshuangxi888]

I believe Attributes#keys() is necessary, especially when performing protocol extensions such as proxy-protocol. In this case, you would write the data from the protocol into Attributes and then retrieve and populate it into Headers within an Interceptor. Due to the presence of uncertain data like TLS in the proxy-protocol, you would need to iterate over Attributes#keys() to retrieve them.

[ejona86]

I believe Attributes#keys() is necessary, especially when performing protocol extensions such as proxy-protocol

No. Just use a single entry in Attributes and store a map as the value for that entry. Then you can loop over the proxy-protocol extensions without looping through all attributes. Not that we support proxy-protocol.

[larry-safran]

June 14 API Stabilization: The deprecated items have been removed. When the comments above are addressed it is approved for stabilization