fix: Multiple REST transport related fixes #997
Conversation
1) Stop failing for grpc+rest and rest transports in case if there are bidi- or client- streaming methods (generated a method throwing "Unimplemented" exception if called) 2) Make sure that the resource name suggested for wildcard resource names actually matches url pattern (most of the changes in goldens are caused by this change) 3) Fix the order of "actual" and "expected" in diff_gen_and_golden to match
… chosen pattern matches url pattern (the changes in `MessagingClient.golden` and related files are caused by this change).
| @@ -55,6 +57,31 @@ public abstract class ResourceName { | |||
|
|
|||
| public abstract boolean isOnlyWildcard(); | |||
|
|
|||
| @Nullable | |||
| public String getMatchingPattern(String binding) { | |||
There was a problem hiding this comment.
Same comment as last method. The logic here also looks much more complicated, so unit tests would be really helpful.
There was a problem hiding this comment.
We don't have tests for Resourcename class directly, but it all gets tested as part of bigger test picture.
There was a problem hiding this comment.
Can we please add a test class for it? This method is pretty complicated and having unit tests will not only help coverage and help us to understand it as well(easier for us to maintain in the future). It may be already tested somewhere but unit test will always fail fast, so that it's much easier to troubleshoot.
| @@ -249,8 +254,10 @@ static Expr createResourceHelperValue( | |||
| List<ResourceName> unexaminedResnames = new ArrayList<>(resnames); | |||
| for (ResourceName resname : resnames) { | |||
| unexaminedResnames.remove(resname); | |||
| if (!resname.isOnlyWildcard()) { | |||
| return createResourceHelperValue(resname, false, unexaminedResnames, fieldOrMessageName); | |||
| if (!resname.isOnlyWildcard() | |||
There was a problem hiding this comment.
I see a couple of new logics in this method, and I understand it's in composers so unit testing it might get complicated, can we add some test cases in the protos if it's not too hard?
There was a problem hiding this comment.
My general feeling about this codebase is that it is beyond comprehension unfortunately and this affects the testing and developing strategy.
This new logic is being tested by the current set of tests already, though it does not have a dedicated unit test item (but most of the code in this huge codebase doesn't)
There was a problem hiding this comment.
I understand the sentiment and I totally feel you when I was developing dynamic routing headers, but do we want to make it worse? e.g. I added this proto to test different combinations for dynamic routing headers, it may still not cover everything but I think it helps.
There was a problem hiding this comment.
I really believe that those extra tests are not making it better, but making it worse. Basically we add more unmaintainable code. This codebase has 10+ times more code than it should for the task it performs. We are just adding unmaintainable tests to unmaintainable code. The gist is covered - this logic affects the existing tests. Testing multiple times the same code does not seem right
…g() method matching pattern to i)
|
@blakeli0 PTAL |
|
Kudos, SonarCloud Quality Gate passed! |
vam-google
changed the title
vam-google
changed the title
1) Stop failing for grpc+rest and rest transports in case if there are bidi- or client- streaming methods (generated a method throwing "Unimplemented" exception if called) 2) Make sure that the resource name suggested for wildcard resource names actually matches url pattern (most of the changes in goldens are caused by this change) 3) Fix the order of "actual" and "expected" in diff_gen_and_golden to match 4) Fix multipattern `of%Name` method resoluiton to make sure that the chosen pattern matches url pattern (the changes in `MessagingClient.golden` and related files are caused by this change). 5) Fix anonymous resource name class construction (add proper toString() method matching pattern) 6) Gracefully treat methods without http bindings as not supported in rest transport (instead of failing on generation)
🤖 I have created a release *beep* *boop* --- ### Updating meta-information for bleeding-edge SNAPSHOT release. --- This PR was generated with [Release Please](https://togithub.com/googleapis/release-please). See [documentation](https://togithub.com/googleapis/release-please#release-please).
of%Namemethod resoluiton to make sure that the chosen pattern matches url pattern (the changes inMessagingClient.goldenand related files are caused by this change).