Skip to content

feat: add grpc-validation-utils module #41

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

Merged
merged 2 commits into from
Dec 7, 2022
Merged

feat: add grpc-validation-utils module #41

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
f2b649b
feat: add grpc-validation-utils module
GurtejSohi Dec 5, 2022
73366fd
Merge branch 'main' into grpc-validation-utils
GurtejSohi Dec 7, 2022
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
9 changes: 9 additions & 0 deletions grpc-validation-utils/build.gradle.kts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
plugins {
`java-library`
}

dependencies {
api(project(":grpc-context-utils"))
api("io.grpc:grpc-api")
Copy link
Contributor Author

@GurtejSohi GurtejSohi Dec 5, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

@aaron-steinfeld This was defined as an api dependency at the place from which this module is copied - https://github.com/hypertrace/config-service/blob/2a36d97d9d50978cc8ea119d6b8f96bd9e0238e5/validation-utils/build.gradle.kts#L7. We're only using io.grpc.Status from this dependency and it is used to throw StatusRuntimeException. But, since none of the public methods declare that it throws that exception, shouldn't it be an implementation dependency?

Copy link
Contributor

Choose a reason for hiding this comment

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

Technically... maybe? But I would leave it since it's intended to be a grpc status exception, it's part of its implicit contract. It's only a runtime exception because it makes it easier to work with in config service.

On the broader move to pull it out of config service and publish it - I'm not really a big fan, but could maybe be convinced. At a high level, this just feels like the DRY plague too me - there's about 40 lines of code that we might want to share, is that worth having to work across multiple repos, maintain library versioning etc? And more over, this generic proto stuff shouldn't really be applying to other repos - config is unique in hosting many unrelated services.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Technically... maybe? But I would leave it since it's intended to be a grpc status exception, it's part of its implicit contract. It's only a runtime exception because it makes it easier to work with in config service.

Got it.

On the broader move to pull it out of config service and publish it - I'm not really a big fan, but could maybe be convinced. At a high level, this just feels like the DRY plague too me - there's about 40 lines of code that we might want to share, is that worth having to work across multiple repos, maintain library versioning etc?

I agree that there's only a few lines of code but my thinking was that almost every grpc service has CRUD operations and each of those services should be doing some kind of validation for those operations (like checking the request context), especially for create and update operations where it should be checking for non-default presence of mandatory fields where these generic methods could come in handy. So, rather than duplicating the code at a lot of places, it's better to have that at a single place only.

Regarding the overhead, I don't think it would be too much because if a repo is dependent on this module, it would most likely be dependent on the server-utils module, and since they belong to this same repo, maintaining versioning and all shouldn't be that difficult. And anyways, it would still be upto the consumer to use this library or not 🙂

And more over, this generic proto stuff shouldn't really be applying to other repos - config is unique in hosting many unrelated services.

Let's say we want to check for the non-default presence of 10 different fields in a proto object. Then, if we don't have such generic methods, we'd be doing a similar kind of thing which is being done in those generic methods at 10 places, which could make the code quite ugly.

Copy link
Contributor

Choose a reason for hiding this comment

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

I agree that there's only a few lines of code but my thinking was that almost every grpc service has CRUD operations and each of those services should be doing some kind of validation for those operations (like checking the request context), especially for create and update operations where it should be checking for non-default presence of mandatory fields where these generic methods could come in handy. So, rather than duplicating the code at a lot of places, it's better to have that at a single place only.

My thinking was that the generated methods like hasXyz or getXyzCount were sufficient, but forgot about primitives, so I buy this.

Regarding the overhead, I don't think it would be too much because if a repo is dependent on this module, it would most likely be dependent on the server-utils module, and since they belong to this same repo, maintaining versioning and all shouldn't be that difficult. And anyways, it would still be upto the consumer to use this library or not

The point here is that if something changes later, then it requires a two step change. So whether they're using server utils or made the initial decision to consume this library or not doesn't really play in. Worse, if there's a bug here then every consuming repo needs to be updated.

Then, if we don't have such generic methods, we'd be doing a similar kind of thing which is being done in those generic methods at 10 places, which could make the code quite ugly.

Yeah, basically same point above. For the non-primitive fields I think it's more readable to do the if(!hasXyz) throw pattern anyway, but given the primitives and default values, I buy this (and then like the consistency of only using one way).

implementation("com.google.protobuf:protobuf-java-util:3.21.7")
}
58 changes: 58 additions & 0 deletions ...tion-utils/src/main/java/org/hypertrace/core/grpcutils/validation/GrpcValidatorUtils.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,58 @@
package org.hypertrace.core.grpcutils.validation;

import com.google.protobuf.Descriptors.FieldDescriptor;
import com.google.protobuf.Message;
import com.google.protobuf.util.JsonFormat;
import io.grpc.Status;
import org.hypertrace.core.grpcutils.context.RequestContext;

public class GrpcValidatorUtils {
private static final JsonFormat.Printer JSON_PRINTER = JsonFormat.printer();

private GrpcValidatorUtils() {}

public static void validateRequestContextOrThrow(RequestContext requestContext) {
if (requestContext.getTenantId().isEmpty()) {
throw Status.INVALID_ARGUMENT
.withDescription("Missing expected Tenant ID")
.asRuntimeException();
}
}

public static <T extends Message> void validateNonDefaultPresenceOrThrow(
T source, int fieldNumber) {
FieldDescriptor descriptor = source.getDescriptorForType().findFieldByNumber(fieldNumber);

if (descriptor.isRepeated()) {
validateNonDefaultPresenceRepeatedOrThrow(source, descriptor);
} else if (!source.hasField(descriptor)
|| source.getField(descriptor).equals(descriptor.getDefaultValue())) {
throw Status.INVALID_ARGUMENT
.withDescription(
String.format(
"Expected field value %s but not present:%n %s",
descriptor.getFullName(), printMessage(source)))
.asRuntimeException();
}
}

private static <T extends Message> void validateNonDefaultPresenceRepeatedOrThrow(
T source, FieldDescriptor descriptor) {
if (source.getRepeatedFieldCount(descriptor) == 0) {
throw Status.INVALID_ARGUMENT
.withDescription(
String.format(
"Expected at least 1 value for repeated field %s but not present:%n %s",
descriptor.getFullName(), printMessage(source)))
.asRuntimeException();
}
}

public static String printMessage(Message message) {
try {
return JSON_PRINTER.print(message);
} catch (Exception exception) {
return message.toString();
}
}
}
1 change: 1 addition & 0 deletions settings.gradle.kts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,3 +17,4 @@ include(":grpc-client-rx-utils")
include(":grpc-server-rx-utils")
include(":grpc-context-utils")
include(":grpc-server-utils")
include(":grpc-validation-utils")