Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

KeysOf<T> and ParentKeysOf<T> create duplicate parameters #462

Closed
nguerrera opened this issue Apr 20, 2022 · 3 comments · Fixed by #1115
Closed

KeysOf<T> and ParentKeysOf<T> create duplicate parameters #462

nguerrera opened this issue Apr 20, 2022 · 3 comments · Fixed by #1115
Assignees

Comments

@nguerrera
Copy link
Contributor

nguerrera commented Apr 20, 2022

Currently, copyResourceKeyParameters creates new properties with different names than the original properties on a new model, but the parent .model still points to the source model. This seems like breaking an invariant and probably needs to be changed, but merely doing so makes the OpenAPI much uglier and more redundant.

This then causes the OpenAPI emitter to generate duplicate parameters and overwrite them. The problem is that there are other circumstances where you can have a duplicate parameter key (for example, with duplicate friendlyNames) where it would be an error to silently overwrite. I'm putting in a workaround to allow the overwriting only if the full JSON representation of the parameter is unchanged, and pointing back to this issue in a comment to find a long term solution.

@markcowl markcowl added this to the Backlog milestone Apr 25, 2022
@markcowl markcowl added the design:needed A design request has been raised that needs a proposal label Apr 25, 2022
@markcowl markcowl modified the milestones: Backlog, [2022] June May 11, 2022
@markcowl markcowl removed the design:needed A design request has been raised that needs a proposal label May 25, 2022
@daviwil
Copy link
Contributor

daviwil commented Jun 2, 2022

@nguerrera I took a look at #506, is my understanding correct that it will help implement this particular PR by not confusing the wrongly-parented key property with a property from the original model type? Will that help with schema naming and inlining?

Any other thoughts on what I might need to do to fix this issue?

@nguerrera
Copy link
Contributor Author

I don't think #506 is enough. I've sadly forgotten the details here, but let's look at this together if you have a moment to get on a call.

@nguerrera
Copy link
Contributor Author

I think the real help for this would be #521. I think it might be reasonable to de-prioritize this until we have that. There is an effective (though ugly) workaround to dedupe the definitions that result in the OpenAPI emitter. The customer impact is maybe not high, but we should address the technical debt at some point and I think it would be easiest if we had #521. That said, there might be another fix that doesn't need that and it would still be goodness to fix it sooner in some easier way if we find one.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
5 participants