Trim the "@-" suffix from resource names before calling the List*Revisions APIs.

[timburks]

This fixes a problem that @theganyo discovered in which calls to hosted versions of the ListApiSpecRevisions and ListApiDeploymentRevisions failed when @- was appended to the resource names. These suffixes were being included in the name arguments, which is strictly expected to be a resource name (and not a pattern that matches collections). The core implementation accidentally supports these modified names, but hosted implementations may not, because it is out of spec, and agreeing with that, we trim the @- from these names before passing them as arguments.

This is a Postel's Law solution -- the visitor package is now conservative in the requests that it generates but the core server is liberal (permissive) in what it accepts. Should we create an issue to tighten that?

[codecov[bot]]

Codecov Report

Merging #1218 (74a4cf2) into main (bce4232) will increase coverage by 0.00%. The diff coverage is 100.00%.

@@           Coverage Diff           @@
##             main    #1218   +/-   ##
=======================================
  Coverage   71.95%   71.96%           
=======================================
  Files         146      146           
  Lines       12239    12241    +2     
=======================================
+ Hits         8807     8809    +2     
  Misses       2749     2749           
  Partials      683      683           

Files Changed	Coverage Δ
pkg/visitor/list.go	73.65% <100.00%> (+0.28%)	:arrow_up:

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

[theganyo]

If I understand this correctly, I disagree with the characterization as well as the solution. Supporting the revision wildcard was intentional. But nevertheless, if we feel we must reverse that feature, I believe we should disable it at a lower level and leave this as-is, for if the "@-" is not a wildcard, returning a 404 would be more correct - as the "-" matches no resource. Stripping the "@-" actually just changes the meaning of "-" to return the latest revision, which only seems to create inconsistency.

[timburks]

Just for clarity, I think we're only concerned with the List*Revisions APIs and the acceptable values that can be passed for the name argument. Reading the "Listing Revisions" section of aip-162, I don't see any mention of the @- pattern for name but I do see this comment referring to name in an example:

  // The name of the book to list revisions for.

That suggests to me that the argument should be a name of a resource and not a pattern matching a revision.

The next section "Child resources" does include a @- pattern but only for listing resources that are below the revisions (in our case, artifacts).

If we modified the core server to treat these suffixes as errors, wouldn't we also want the client to stop sending these patterns for the name argument of List*Revisions? (I'm not seeing why addressing this on the server would block this change)

[theganyo]

Ah, I see. My mistake. Maybe this will teach me to review PRs on the weekend, but I doubt it. :)