feat: add grpc-validation-utils module

[GurtejSohi]

Description

Moving GrpcValidatorUtils from config-service to here, so that it can be consumed by other services also, as a library.

[codecov]

Codecov Report

Merging #41 (73366fd) into main (27870c2) will not change coverage.
The diff coverage is n/a.

@@            Coverage Diff            @@
##               main      #41   +/-   ##
=========================================
  Coverage     73.59%   73.59%           
  Complexity      145      145           
=========================================
  Files            20       20           
  Lines           409      409           
  Branches         22       22           
=========================================
  Hits            301      301           
  Misses           85       85           
  Partials         23       23           

Flag	Coverage Δ
unit	73.59% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

[GurtejSohi]

@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?

[aaron-steinfeld]

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.

[GurtejSohi]

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.

[aaron-steinfeld]

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).

[github-actions]

Unit Test Results

12 files  ±0  12 suites  ±0   15s ⏱️ -1s
70 tests ±0  70 ✔️ ±0  0 💤 ±0  0 ❌ ±0 

Results for commit bb31fd8. ± Comparison against base commit 27870c2.