redefine IoT Core custom authorizer request and response structs #401
Conversation
events/iot.go
Outdated
| PolicyDocuments []*IoTCorePolicyDocument `json:"policyDocuments"` | ||
| } | ||
|
|
||
| type IoTCorePolicyDocument struct { |
There was a problem hiding this comment.
This model duplicates APIGatewayCustomAuthorizerPolicy.
There was a problem hiding this comment.
I feel it's pretty strange to have the IoT Core authorizer struct have a dependency on the API Gateway struct. Even if they are the same struct, the names are rather jarring.
What I will do is make a copy of APIGatewayCustomAuthorizerPolicy named IAMPolicyDocument and make the former an alias of the latter. That way we don't break backwards compatibility, but new events aren't forced to reference an API Gateway struct.
| // | ||
| // Deprecated: This type exists for backwards compatibility. New events | ||
| // and structs should reference IAMPolicyDocument directly instead. | ||
| type APIGatewayCustomAuthorizerPolicy IAMPolicyDocument |
There was a problem hiding this comment.
To keep backcompat, I believe this would need to be a type alias. eg: write as type APIGatewayCustomAuthorizerPolicy = IAMPolicyDocument
There was a problem hiding this comment.
That is not necessary for backwards compatibility. type A B makes A a copy of B (same struct fields), they just are not equivalent/interchangeable. type A = B makes A an alias of B, meaning they are interchangeable (e.g., type byte = uint8). Unless you want clients to be able to use an APIGatewayCustomAuthorizerPolicy wherever an IAMPolicyDocument is expected without casting, there is no need for an alias.
There was a problem hiding this comment.
my brain read the diff as having APIGatewayCustomAuthorizerResponse also being updated to use IAMPolicyDocument for it's PolicyDocument field, which would have broken something like x.PolicyDocument = APIGatewayCustomAuthorizerPolicy{} (see: https://play.golang.org/p/_OXhN9ebJ1K). I see now that APIGatewayCustomAuthorizerResponse wasn't updated. I can't tell if that was intentional or not?
If you think that APIGatewayCustomAuthorizerResponse shouldn't be updated, then lets remove the //Deprecated: comment, as it's use of APIGatewayCustomAuthorizerPolicy would still be correct in that context
There was a problem hiding this comment.
The "deprecated" comment was directed more at API developers (to tell them not to use it in new structs), not API clients.
I had not intended to make changes to APIGatewayCustomAuthorizerResponse but I can do that if you prefer.
There was a problem hiding this comment.
I think to just drop the comment so that no one's linter freaks out. DRYing the rest api gateway type feels out of scope to me for this PR
| } | ||
|
|
||
| // IoTCustomAuthorizerResponse represents the expected format of an IoT device gateway authorization response. | ||
| type IoTCustomAuthorizerResponse struct { |
There was a problem hiding this comment.
I like the rename. To keep strict compile time backwards compatibility, I'd like for the old types to be left in the project too. They can have a comment like // Deprecated does not model schema correctly, use XYZType instead. They might also be moved to a file like iot_deprecated.go to help with the clutter.
There was a problem hiding this comment.
Let's not put loadbearing invariants in comments please, as they are easy to overlook.
There was a problem hiding this comment.
@bmoffatt Is it really necessary to keep the original struct? No one could be using it in practice since the PolicyDocuments field had the wrong type, and without that you won't have a particularly useful authorizer.
There was a problem hiding this comment.
Yeah I wanna keep the old ones, type changes and deletions are breaking changes. This isn't the first time things have been gotten wrong, and so far this project has not respond to that by deleting code. I'm not satisfied with incorrect models hanging around either, and i'd like to try and solve that holistically rather than making possibly inconsistent actions on a case-by-case basis
Let's not put loadbearing invariants in comments please, as they are easy to overlook.
The guidance comes from https://go.dev/blog/godoc
To signal that an identifier should not be used, add a paragraph to its doc comment that begins with “Deprecated:” followed by some information about the deprecation.
My IDE renders these with a helpful strikethrough. The form can also be captured by the popular staticcheck linter
There was a problem hiding this comment.
i'd like to try and solve that holistically rather than making possibly inconsistent actions on a case-by-case basis
Really I think this library should have made a separate module for each service. That way clients can just include the events they need rather than using a single ever-growing events package. This would also reduce some of the repetition in the typedefs (e.g., iotcore.CustomAuthorizerResponse vs. events.IoTCoreCustomAuthorizerResponse), and would allow the event structs for each service to be versioned independently.
For example in this situation we could have just made a new major rev of the IoT Core events module rather than having to (ultimately pointlessly) maintain backwards compatibility.
There was a problem hiding this comment.
Thanks for the feedback! I think it highlights some obvious shortcomings of the approach this library went with. I'll forward this on to the product team.
The approach you described is actually how it was just before the initial public release. The monolithic events package approached was chosen after receiving feedback from our private preview customers around poor event type discovery with IDE auto-complete suggestions at the time.
|
Hello everybody, I stumbled on to the same issue. Is there anyway we can proceed with this? |
|
made some changes to the deprecation notices |
Fixes #400
Description of changes:
Redefined the IoT Core custom authorizer request and response structs to match the real spec instead of the test spec.
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.
cc @bmoffatt