API: Update oapi-codegen version to v1.12.0 of our fork #4707
Conversation
Updates the version of oapi-codegen which is installed via the buildtools to our v1.11.0 prerelease. Also changes the Makefile targets and adds config files which are the preferred way of configuration instead of the old CLI flags. The generated types definitions have also been updated using the oapi-codegen commit which ensures all required Enums are generated for enums embedded in response types.
Eric-Warehime
added
the
tech debt
Codecov Report
@@ Coverage Diff @@
## master #4707 +/- ##
==========================================
+ Coverage 54.55% 54.61% +0.06%
==========================================
Files 414 414
Lines 53646 53646
==========================================
+ Hits 29268 29301 +33
+ Misses 21947 21921 -26
+ Partials 2431 2424 -7
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
|
The reason I liked the "unknown param" feature is because it's easy to make a typo in a query parameter. This would cause us to silently ignore the problem instead of making the caller fix it. I probably added this feature after troubleshooting a bug only to eventually realize there was a typo and everything was working correctly. |
Eric-Warehime
changed the title
|
I've updated the PR to point to v1.12.0 instead of v1.11.0 since they've just released 1.12.0 in the last few days. There are also some features in the newer version that will allow us to consolidate the duplicated types code into a single location. There are a couple of changes to the model types from this--mostly around enums/typedefs. |
daemon/algod/api/server/v2/utils.go
Outdated
| fallthrough | ||
| case "msgp": | ||
| // Can this actually ever happen? |
There was a problem hiding this comment.
It looks like we should keep this for backwards compatibility - a client could call an API with the format set to "msgp" expecting a msgpack response. But it looks like this is never tested in the code in handler_test.go.
| golang.org/x/sys v0.0.0-20220319134239-a9b59b0215f8 | ||
| golang.org/x/text v0.3.7 | ||
| github.com/stretchr/testify v1.8.1 | ||
| golang.org/x/crypto v0.1.0 |
There was a problem hiding this comment.
@Eric-Warehime The PR presents a number of dependency version changes beyond oapi-codegen. How should the changes be interpreted?
- I'm trying to gauge if all changes are necessary. And for necessary changes, I'm gauging the level of review performed.
- I'm asking without (yet) having researched specific changes.
There was a problem hiding this comment.
These are all packages which are either direct or indirect dependencies of oapi-codegen. The ones in this section are also direct dependencies of go-algorand.
In terms of upgrading them, it's just a result of running go get github.com/algorand/oapi-codegen@v1.12.0-algorand.0. I haven't personally gone through every one of them to examine the changelogs. Most of the direct dep updates relate to our API (echo, mux, kin), testify is a test dep, and the rest are standard go modules.
It seems to me that upgrading is purely to our advantage for these sorts of things. For example, here is the diff for our crypto dep which shows 5 security vulnerabilities coming from transitive dependencies https://deps.dev/go/golang.org%2Fx%2Fcrypto/v0.0.0-20220321153916-2c7772ba3064/compare?v2=v0.1.0
Let me know if you want me to look into any of these more and I'm happy to.
There was a problem hiding this comment.
@Eric-Warehime Got it - I'm open to being told I'm off-base, but my current thinking is:
- For any dependency used in transaction processing, I think we must closely review diffs for unexpected behavior changes.
- During this pass, I didn't have time to vet dependencies. I'll aim to do so, but I want to leave the note in case there's different opinions + so that others can independently verify.
Rationale:
- Generally, I agree with your perspective - Stale dependencies are often problematic.
- However, for transaction processing logic, I think we must confirm that APIs go-algorand already uses do not have behavior changes.
- If we missed a behavior change, it's possible that node 1 running dependency A@v1.0.0 disagrees with node 2 running A@v1.1.0.
- Here's a previous example that's heightened my attention to the concern: AVM: Catch any panic in edcsa verifying #4368 (comment).
There was a problem hiding this comment.
Checking the following packages seems reasonable:
- golang.org/x/crypto
- golang.org/x/sys
- golang.org/x/text
- github.com/gorilla/mux
golang.org/x/crypto
I don't see commits that touch any of the packages we're using.
https://github.com/golang/crypto/commits/master
golang.org/x/sys
This is for OS specific helpers. We're mostly OS agnostic, in the cases we aren't I don't see a lot of overlap in the changes to this library.
https://github.com/golang/sys/commits/master?before=fc697a31fa06b616162e34fd66047ab52722ba6c+35&branch=master
golang.org/x/text
This is only used in test code for data/transactions/logic/jsonspec_test.go
There are also very few changes in the history and I don't see anything relevant to what we're using.
https://github.com/golang/text/commits/master
github.com/gorilla/mux
This definitely has the most changes, but they seem to be mostly new features and performance improvements. Out of these 4 this is probably the biggest risk.
https://github.com/gorilla/mux/releases
https://github.com/gorilla/mux/commits/master
There was a problem hiding this comment.
@Eric-Warehime I also performed a review and found no concerns for transaction processing. If you're comfortable with the reviews performed by @winder and me, then feel welcomed to resolve the thread.
Methodology:
- For each dependency below, I reviewed release notes and associated commit messages.
- I scanned commit messages looking for changes to areas of interest, dependency changes, and/or breaking changes.
- In my review, I did not identify changes worth raising for further review. Admittedly, echo dependency changed a lot. I'm mostly trusting that passing tests + no obvious commits for breaking behavior change impacting v4.1.17 mean no regression here.
There was a problem hiding this comment.
@Eric-Warehime Thanks for the effort to bring in upstream updates - looks ready to me.
RE: Removal of query string parameter validation (oapi-codegen/oapi-codegen@9cd29e2):
- It feels to me that the validation is nice to have though I presume removal came after moderate attempts to retain it.
- Presuming that's the case, I don't know enough about the history to know how strongly to feel. I'm trusting that if other more experienced reviewers are OK with the change, then we're good to go.
| @@ -31,5 +31,5 @@ call_and_verify "App parameter parsing error 1." "/v2/applications/-2" 400 "Inva | |||
| call_and_verify "App parameter parsing error 2." "/v2/applications/not-a-number" 400 "Invalid format for parameter application-id" | |||
|
|
|||
| # Good request, but invalid query parameters | |||
| call_and_verify "App invalid parameter" "/v2/applications/$APPID?this-should-fail=200" 400 'Unknown parameter detected: this-should-fail' | |||
| call_and_verify "App invalid parameter" "/v2/applications/$APPID?this-should-not-fail=200" 200 '"global-state-schema":{"num-byte-slice":0,"num-uint":2}' | |||
There was a problem hiding this comment.
does /v2/applications/$APPID and /v2/assets/$ASSET_ID return 200 for invalid params?
There was a problem hiding this comment.
The update removes this commit oapi-codegen/oapi-codegen@9cd29e2 from our fork. So the call will return the status that occurs when you execute the API, but ignore the query parameters that don't correspond to a valid parameter value.
|
I've pushed to a branch on go-algorand and triggered the nightly tests on it to validate that they pass (or that they fail w/ the same issues as the current nightly branch) https://app.circleci.com/pipelines/github/algorand/go-algorand/10399/workflows/ca59b6d5-7390-40f0-98fe-018d77634ed0 |
|
Had some failures in the nightly tests...it looks like they were caused by ae443cd and fixed in 0eae1c0 I've merged upstream and am re-running nightly tests in https://app.circleci.com/pipelines/github/algorand/go-algorand/10436/workflows/a9f72ca1-d797-4bce-af16-49437769c492 |
Eric-Warehime
dismissed stale reviews from shiqizng, michaeldiamant, and winder
via
809c935
winder
changed the title
Summary
This change updates the version of oapi-codegen to v1.12.0 of our forked repo. This includes a bunch of changes to the capabilities of the code generator that will be useful in the (near) future for things like eliminating code duplication by splitting the types definitions into a single common package, and enabling/disabling API endpoints based on the launch parameters of the node. Those changes will come in separate PRs--this PR aims to update our generator with as few changes as possible.
I've updated our Makefile and targets along with adding configs to setup the different generator targets. This is the new preferred method of configuration as opposed to the old version's CLI flags.
There are some changes being made to the generated code--mostly around how the types are generated--that required changes to the handlers, but won't produce functional changes on the API behavior.
Test Plan
Existing unit tests should pass--in theory this is a no-op to our APIs so everything should be backwards compatible with existing tests.
CAVEAT:
The one behavior change that has actually caused me to update some tests here is removing this commit from the code generator. The effect of this is that when an API request is made to algod with query parameters that are unknown we will ignore them where previously this would have caused us to return a 400.
Feel free to let me know why this is a bad change to make if you feel strongly about it.
Also if you want to take a look at the actual oapi-codegen changes involved in this update please see this PR: algorand/oapi-codegen#1