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

Fields included via $allOf into AsyncAPI messages are not constrained correctly #1611

Closed
DavidGregory084 opened this issue Nov 16, 2023 · 7 comments
Labels
bug Something isn't working

Comments

@DavidGregory084
Copy link

DavidGregory084 commented Nov 16, 2023

Describe the bug

I have some shared fields that are used in multiple messages in my schema:

components:
  schemas:
    AbstractData:
      description: Abstract fields
      required:
        - capturedAt
        - deviceId
      properties:
        deviceId:
          type: string
          format: uuid
        capturedAt:
          type: string
          format: date-time

When these are included into my message...

  messages:
    ConcreteData:
      description: Concrete implementation as a message
      payload:
        x-parser-schema-id: ConcreteData
        type: object
        additionalProperties: false
        allOf:
          - $ref: "#/components/schemas/AbstractData"
        required:
          - someValue
        properties:
          someValue:
            type: number

then the generated code does not constrain the fields correctly:

data class ConcreteData(
    val someValue: Double,
    val deviceId: String,
    val capturedAt: String,
)

If I comment out the deviceId field, then the capturedAt field is represented correctly as an OffsetDateTime:

data class ConcreteData(
    val someValue: Double,
    val capturedAt: java.time.OffsetDateTime,
)

How to Reproduce

Please see this Codesandbox or gist.

Expected behavior

I expect that fields will be constrained according to their format, rather than a single field's constraints being applied to every field of that type regardless of the format option. I was attempting to use typeMapping to set custom types for these fields when I noticed this issue:

  typeMapping: {
    String: (opts) => {
      const { constrainedModel } = opts;
      switch (constrainedModel.options.format) {
        case "date-time": {
          return "java.time.Instant";
        }
        case "uuid": {
          return "java.util.UUID";
        }
        default: {
          return KotlinDefaultTypeMapping.String(opts);
        }
      }
    },
  },
@DavidGregory084 DavidGregory084 added the bug Something isn't working label Nov 16, 2023
Copy link
Contributor

Welcome to AsyncAPI. Thanks a lot for reporting your first issue. Please check out our contributors guide and the instructions about a basic recommended setup useful for opening a pull request.
Keep in mind there are also other channels you can use to interact with AsyncAPI community. For more details check out this issue.

@kennethaasan
Copy link
Collaborator

Hi, I was trying to reproduce your issue, but it seems fine here: https://github.com/asyncapi/modelina/pull/1614/files
Would you be able to reproduce this problem in a test?

@DavidGregory084
Copy link
Author

DavidGregory084 commented Nov 17, 2023

@kennethaasan there is a link above to a CodeSandbox which reproduces the issue - you shouldn't have to log in or sign up for anything to view it.
image

Perhaps it's been fixed on main? That would be great if so!

@DavidGregory084
Copy link
Author

There is a difference between your test and the example I provided - the use of @asyncapi/parser to parse the doc. Perhaps this issue actually belongs there?

@kennethaasan
Copy link
Collaborator

Sounds like something for @jonaslagoni to take a look at

@DavidGregory084
Copy link
Author

DavidGregory084 commented Nov 17, 2023

Hmm I was able to replicate the issue with this commit, however while experimenting with it I realised that the issue is triggered by a missing type: object declaration in the AbstractData schema.

I was surprised that none of the tooling told me about this, but apparently a schema without a type is still valid, leading to nonsensical results like this:

image

See this issue for more details.

Let's close this with lesson learned: don't forget the type keyword in JSON schema if you're afraid of nasal demons.

@kennethaasan
Copy link
Collaborator

Perhaps something for the parser to alert us about @jonaslagoni ?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

2 participants