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

Specifying interface in operation props causes emitter to crash #3504

Closed
3 tasks done
shiron-dev opened this issue Jun 3, 2024 · 4 comments · Fixed by #3574
Closed
3 tasks done

Specifying interface in operation props causes emitter to crash #3504

shiron-dev opened this issue Jun 3, 2024 · 4 comments · Fixed by #3574
Assignees
Labels
bug Something isn't working emitter:openapi3 Issues for @typespec/openapi3 emitter triaged:core
Milestone

Comments

@shiron-dev
Copy link
Contributor

Clear and concise description of the problem

When using “@typespec/openapi3”, specifying interface in operation props causes emitter to crash.
From a light reading of the implementation, it seems to me that it is not intended to specify an Interface in this ModelProperty section. In this case, why not make it an error message to the effect that “Interface cannot be specified in ModelProperty”?

Here is a simple example: both TestInterface1 and TestInterface2 generate the same error.
https://typespec.io/playground?c=aW50ZXJmYWNlIFRlc3RJyA4xIHsKICBnZXQxKHByb3A6zx4p0BE7Cn0KCtdMMiB7fQoKb3DETzLUTzLQTzI7Cg%3D%3D&e=%40typespec%2Fopenapi3&options=%7B%7D

Here is the current hard to understand error.

ExternalError: Emitter "@typespec/openapi3" crashed! This is a bug.
Please file an issue at https://github.com/microsoft/typespec/issues

Error: Unexpected non-code result from emit reference
    at OpenAPI3SchemaEmitter.modelPropertyLiteral (https://typespec.blob.core.windows.net/pkgs/@typespec/openapi3/0.56.0/index.js:3866:19)
    at https://typespec.blob.core.windows.net/pkgs/@typespec/compiler/0.56.0/emitter-utils-Bonv920s.js:17245:55
    at withTypeContext (https://typespec.blob.core.windows.net/pkgs/@typespec/compiler/0.56.0/emitter-utils-Bonv920s.js:17388:9)
    at invokeTypeEmitter (https://typespec.blob.core.windows.net/pkgs/@typespec/compiler/0.56.0/emitter-utils-Bonv920s.js:17235:9)
    at Object.emitModelProperty (https://typespec.blob.core.windows.net/pkgs/@typespec/compiler/0.56.0/emitter-utils-Bonv920s.js:17187:20)
    at OpenAPI3SchemaEmitter.modelProperties (https://typespec.blob.core.windows.net/pkgs/@typespec/openapi3/0.56.0/index.js:3825:41)
    at https://typespec.blob.core.windows.net/pkgs/@typespec/compiler/0.56.0/emitter-utils-Bonv920s.js:17245:55
    at withTypeContext (https://typespec.blob.core.windows.net/pkgs/@typespec/compiler/0.56.0/emitter-utils-Bonv920s.js:17388:9)
    at invokeTypeEmitter (https://typespec.blob.core.windows.net/pkgs/@typespec/compiler/0.56.0/emitter-utils-Bonv920s.js:17235:9)
    at Object.emitModelProperties (https://typespec.blob.core.windows.net/pkgs/@typespec/compiler/0.56.0/emitter-utils-Bonv920s.js:17178:25)

Checklist

  • Follow our Code of Conduct
  • Read the docs.
  • Check that there isn't already an issue that request the same feature to avoid creating a duplicate.
@timotheeguerin timotheeguerin added the bug Something isn't working label Jun 3, 2024
@timotheeguerin
Copy link
Member

This shouldn't crash but what would you expect this to do?

@markcowl
Copy link
Contributor

markcowl commented Jun 3, 2024

  • consider handling this in the emitter framework when operations are processed

@markcowl markcowl added this to the Backlog milestone Jun 3, 2024
@markcowl markcowl added the emitter:openapi3 Issues for @typespec/openapi3 emitter label Jun 3, 2024
@shiron-dev
Copy link
Contributor Author

shiron-dev commented Jun 4, 2024

I want the error to be output in the following form. Note that the content of the message is tentative.

/Users/user/projects/typespec/packages/samples/scratch/main.tsp:7:9 - error @typespec/openapi3/model-property-non-code-result: 'Interface' cannot be specified in ModelProperty.
> 7 | op get2(prop: TestInterface2): TestInterface2;
    |         ^^^^
/Users/user/projects/typespec/packages/samples/scratch/main.tsp:2:8 - error @typespec/openapi3/model-property-non-code-result: 'Interface' cannot be specified in ModelProperty.
> 2 |   get1(prop: TestInterface1): TestInterface1;
    |        ^^^^

Found 2 errors.

This was reproduced by calling reportDiagnostic on the part that throws the original error (“Unexpected non-code result from emit reference”). Of course, if the syntax is not intended to specify an Interface in the ModelProperty in the first place, the framework should handle it.
If you are okay with this policy, please let me create a PR.

@timotheeguerin
Copy link
Member

yep that's exactly what I would expect to happen, I was just wondering if you were expecting this to produce some OpenAPI3 output.
Looking forward to the PR

github-merge-queue bot pushed a commit that referenced this issue Jun 13, 2024
fix #3504

When unsuitable types are specified in the model property, the invalid
code location is now reported instead of throwing an error.

When compiling the following tsp file, it is reported as follows
```tsp
interface TestInterface1 {
  get1(prop: TestInterface1): TestInterface1;
}
```

```
Diagnostics were reported during compilation:

/projects/typespec/packages/samples/scratch/main.tsp:2:8 - error @typespec/openapi3/invalid-model-property: 'Interface' cannot be specified as a model property.
> 2 |   get1(prop: TestInterface1): TestInterface1;
    |        ^^^^

Found 1 error.
```

---------

Co-authored-by: Timothee Guerin <tiguerin@microsoft.com>
Co-authored-by: Timothee Guerin <timothee.guerin@outlook.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working emitter:openapi3 Issues for @typespec/openapi3 emitter triaged:core
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants