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

Fix for GET with projections #819

Merged
merged 6 commits into from
Aug 8, 2022
Merged

Conversation

bterlson
Copy link
Member

@bterlson bterlson commented Aug 6, 2022

Tentative fix #812, as I ran out of time to fix it properly. The root cause of this issue seems to be that getEffectiveType doesn't respect projections. The fix I think we should do is to update gET to consult the program's current projection and map the type there. I think @nguerrera should comment on a proper fix, so this is a minimally invasive and somewhat garbage-y wrapper around gET that projects any return value.

@azure-pipelines
Copy link

You can try these changes at https://cadlplayground.z22.web.core.windows.net/prs/819/

@daviwil
Copy link
Contributor

daviwil commented Aug 8, 2022

I think this fix is headed in the right direction because it does fix the issue I reported in #812 (at least for the "Language Authoring" spec, but after testing it a with the Azure.Core versioning PR (https://github.com/Azure/cadl-azure/pull/1754), I see the following error while running the cadl-samples/data-plane/formrecognizer spec:

Cadl compiler v0.33.0

Internal compiler error!
File issue at https://github.com/microsoft/cadl

Error: Namespace FormRecognizer should have already been projected.
    at compilerAssert (file:///home/daviwil/Projects/Code/cadl/versioning/core/packages/compiler/core/diagnostics.ts:293:9)
    at projectType (file:///home/daviwil/Projects/Code/cadl/versioning/core/packages/compiler/core/projector.ts:100:9)
    at projectedNamespaceScope (file:///home/daviwil/Projects/Code/cadl/versioning/core/packages/compiler/core/projector.ts:510:12)
    at shallowClone (file:///home/daviwil/Projects/Code/cadl/versioning/core/packages/compiler/core/projector.ts:555:30)
    at projectModel (file:///home/daviwil/Projects/Code/cadl/versioning/core/packages/compiler/core/projector.ts:250:28)
    at Object.projectType (file:///home/daviwil/Projects/Code/cadl/versioning/core/packages/compiler/core/projector.ts:107:21)
    at Object.getProjectedEffectiveModelType [as getEffectiveModelType] (file:///home/daviwil/Projects/Code/cadl/versioning/core/packages/compiler/core/checker.ts:4122:52)
    at getEffectiveSchemaType (file:///home/daviwil/Projects/Code/cadl/versioning/packages/cadl-autorest/src/openapi.ts:678:41)
    at getSchemaOrRef (file:///home/daviwil/Projects/Code/cadl/versioning/packages/cadl-autorest/src/openapi.ts:631:12)
    at getSchemaForModel (file:///home/daviwil/Projects/Code/cadl/versioning/packages/cadl-autorest/src/openapi.ts:1163:26)

It seems like we might need to investigate the source of this error before we can merge this PR. The weird thing is that I can't really even tell by looking at the Form Recognizer spec what might be triggering this.

We're in luck, though: this test is failing with the same error so we might be able to clamp down using that test and figure out why the error occurs.

@bterlson bterlson changed the title tentative dirty fix for GET with projections Fix for GET with projections Aug 8, 2022
@markcowl markcowl merged commit 611e4fb into microsoft:main Aug 8, 2022
@nguerrera
Copy link
Contributor

I think @nguerrera should comment on a proper fix, so this is a minimally invasive and somewhat garbage-y wrapper around gET that projects any return value.

I missed this while I was away and am hitting something related now. It seems suspect that getEffectiveModelType is finding unprojected types for a projected type. It appears that we are not fixing up parent .model and .sourceProperty when projecting. However, so far when I do that more things break :(

@nguerrera
Copy link
Contributor

#1069 tracks this now

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

Successfully merging this pull request may close these issues.

Templated "filter types" used in versioned namespaces create duplicate types
5 participants