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.

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

core: add accessor for bare method name in MethodDescriptor #7339

Merged
merged 3 commits into from Sep 3, 2020

Conversation

pkern
Copy link
Contributor

@pkern pkern commented Aug 19, 2020

To implement credential restrictions based on not only service names but also (non-qualified) method names, we need to expose this property just like the service name got exposed through the API.

This reuses the same experimental tracking issue as getServiceName, but I can to file a new one if necessary.

@linux-foundation-easycla
Copy link

linux-foundation-easycla bot commented Aug 19, 2020

CLA Check
The committers are authorized under a signed CLA.

@@ -225,6 +226,7 @@ private MethodDescriptor(
this.type = Preconditions.checkNotNull(type, "type");
this.fullMethodName = Preconditions.checkNotNull(fullMethodName, "fullMethodName");
this.serviceName = extractFullServiceName(fullMethodName);
this.methodName = extractMethodName(fullMethodName);
Copy link
Member

Choose a reason for hiding this comment

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

I don't think we should pre-compute this.

Copy link
Contributor

Choose a reason for hiding this comment

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

Does this really matter? This should be a fairly cheap operation.

Copy link
Member

Choose a reason for hiding this comment

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

If it's so cheap we shouldn't mind doing it every time. Really, I don't think we want the startup initialization overhead and memory usage for something that may never be used (consider Android for example).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

My concern is that then every caller will need to cache it, but alas. Done.

*/
@Nullable
@ExperimentalApi("https://github.com/grpc/grpc-java/issues/5635")
public String getMethodName() {
Copy link
Member

Choose a reason for hiding this comment

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

We should add something to the name to make users think whether they want the full name or this one. "simple" or "bare" or similar. Ditto for extract method.

I'm not sure if we have any such term today, as method name is normally fully qualified. Maybe another language has such a thing?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I am not aware of another language having solved this properly. The naming confusion is very real. Generally the full(y qualified) method name gets its own name (also here: getFullMethodName()) and the other one is just MethodName. But from the options I picked Bare for now. Let me know what you think.

* @since 1.32.0
*/
@Nullable
public static String extractMethodName(String fullMethodName) {
Copy link
Member

Choose a reason for hiding this comment

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

ExperimentalApi

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

@voidzcy voidzcy changed the title core: add getMethodName to MethodDescriptor core: add getBareMethodName to MethodDescriptor Sep 3, 2020
@voidzcy voidzcy changed the title core: add getBareMethodName to MethodDescriptor core: add accessor for bare method name in MethodDescriptor Sep 3, 2020
@voidzcy voidzcy merged commit 0701242 into grpc:master Sep 3, 2020
dfawley pushed a commit to dfawley/grpc-java that referenced this pull request Jan 15, 2021
Added getBareMethodName and extractBareMethodName for accessing the unqualified method name.
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jun 8, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants