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

Traits break comparisons and calls for v3 and v2 #856

Closed
Tracked by #953
jonaslagoni opened this issue Sep 1, 2023 · 7 comments
Closed
Tracked by #953

Traits break comparisons and calls for v3 and v2 #856

jonaslagoni opened this issue Sep 1, 2023 · 7 comments
Labels
bug Something isn't working

Comments

@jonaslagoni
Copy link
Member

Describe the bug

I only have one concrete instance to show for here, but I bet it applies to other places as well.

How to Reproduce

With the following example https://github.com/asyncapi/spec/blob/next-major-spec/examples/streetlights-kafka.yml

And if you iterate over operations and want to find the associate channel for the operation:

operations.forEach(operation => {
    const channel = operation.channels().all()[0];
    // Channel is always undefined when traits are used
    ...

It will always fail to find the channel if traits have been applied. The problem is the following line:

if (channel === this._json.channel) {

An example would be, channel would hold the value:

{
  address: "smartylighting.streetlights.1.0.event.{streetlightId}.lighting.measured",
  messages: {
    "receiveLightMeasurement.message": {
      headers: {
        type: "object",
        properties: {
          "my-app-header": {
            type: "integer",
            minimum: 0,
            maximum: 100,
            "x-parser-schema-id": "<anonymous-schema-3>",
          },
        },
        "x-parser-schema-id": "<anonymous-schema-2>",
      },
      name: "lightMeasured",
      title: "Light measured",
      summary: "Inform about environmental lighting conditions of a particular streetlight.",
      contentType: "application/json",
      traits: [
        {
          headers: {
            type: "object",
            properties: {
              "my-app-header": {
                type: "integer",
                minimum: 0,
                maximum: 100,
                "x-parser-schema-id": "<anonymous-schema-25>",
              },
            },
            "x-parser-schema-id": "<anonymous-schema-24>",
          },
        },
      ],
      payload: {
        type: "object",
        properties: {
          lumens: {
            type: "integer",
            minimum: 0,
            description: "Light intensity measured in lumens.",
            "x-parser-schema-id": "<anonymous-schema-5>",
          },
          sentAt: {
            type: "string",
            format: "date-time",
            description: "Date and time when the message was sent.",
            "x-parser-schema-id": "<anonymous-schema-6>",
          },
        },
        "x-parser-schema-id": "<anonymous-schema-4>",
      },
    },
  },
  description: "The topic on which measured values may be produced and consumed.",
  parameters: {
    streetlightId: {
      description: "The ID of the streetlight.",
    },
  },
}

and this._json.channel would contain:

{
  address: "smartylighting.streetlights.1.0.event.{streetlightId}.lighting.measured",
  messages: {
    "receiveLightMeasurement.message": {
      name: "lightMeasured",
      title: "Light measured",
      summary: "Inform about environmental lighting conditions of a particular streetlight.",
      contentType: "application/json",
      traits: [
        {
          headers: {
            type: "object",
            properties: {
              "my-app-header": {
                type: "integer",
                minimum: 0,
                maximum: 100,
                "x-parser-schema-id": "<anonymous-schema-25>",
              },
            },
            "x-parser-schema-id": "<anonymous-schema-24>",
          },
        },
      ],
      payload: {
        type: "object",
        properties: {
          lumens: {
            type: "integer",
            minimum: 0,
            description: "Light intensity measured in lumens.",
          },
          sentAt: {
            type: "string",
            format: "date-time",
            description: "Date and time when the message was sent.",
          },
        },
      },
    },
  },
  description: "The topic on which measured values may be produced and consumed.",
  parameters: {
    streetlightId: {
      description: "The ID of the streetlight.",
    },
  },
}

Which should return true in comparison, however it will never happen because the underlying json are different because the parser applied traits, and in theory the extensions as well.

@jonaslagoni jonaslagoni added the bug Something isn't working label Sep 1, 2023
@jonaslagoni
Copy link
Member Author

/progress 10 started to look into this...

@jonaslagoni
Copy link
Member Author

/progress 25 starting to get a hang of it and the different solutions, still need to figure out the best approach.

@jonaslagoni
Copy link
Member Author

jonaslagoni commented Oct 24, 2023

Soo, this issue actually digs deeper than just on the surface... And the "problem" is different based on how you view the underlying implementation.

Current behaviour

When we apply the different "operations" such as parsing schemas, applying traits, anonymous naming, and handling circular references, we apply these things to two models document and detailed.

  • checkCircularRefs -> document
  • applyTraitsV3 -> detailed.parsed
  • parseSchemasV3 -> detailed.parsed
  • resolveCircularRefs -> document
  • anonymousNaming -> document

Even though these two are based on the same underlying in-memory object, the way the operations are applied, sometimes overwrites the entire object, meaning we will have a mismatch between the underlying data, where one gets the information and another wont.

So when we in channels try to iterate over this._meta.asyncapi?.parsed.channels and compare it with the locally stored channel this._json.channel they will always mismatch. This is because this._meta.asyncapi?.parsed.channels will have anonymousNaming and circular references marked but this._json.channel will not.

Solutions

These are the solutions I can come up with, feel free to jump in with other approaches!

Temporary fix

We could create a temporary fix JUST for solving the traits. One way to do this is to mark each object (probably channel, message, and operation) with unique IDs and then compare them instead of the entire JSON object.

I don't have an overview, but if we have multiple places in the models where we compare the JSON structure of the two JSON variants, we might have more bugs somewhere. So I don't think that fully solves the underlying problem.

Unifying the underlying JSON

We could unify the underlying JSON, meaning that the two properties don't differ at all. In theory that should work, without having to change the underlying models.

However, I still think it's bad practice to compare entire JSON instead of simple unique ID's. Also, I don't know if it will work with circular references 🤔

Temporary + fix the underlying JSON

We could also combine the temporary with fixing the underlying JSON, as it's probably a better approach regardless 🤔

@smoya
Copy link
Member

smoya commented Oct 25, 2023

Do not take this as a proper answer. I need more time to refresh the whole workflow for parsing and applying custom ops, etc. Only then, I will come back and give you a better answer and hopefully help taking a decision.

In one side, Unifying the underlying JSON seems like something I would expect to behave by default. If there is no strong reason to keep different versions of "same" object, I see then no reason to keep it in that way then.
cc @magicmatatjahu you have all the historical knowledge on that side, would you mind helping us clarifying this? Just in case there is some use case we are missing.

About the unique ID thing, I tried to find an old discussion we had somewhere in an issue where we discussed just a bit about that concept; to have that uid in all objects so we could compare. I would like to remember what were the caveats but I can't. I think @magicmatatjahu shared them as well 🤔

@jonaslagoni
Copy link
Member Author

/progress 40 problem + potential solutions have been found, waiting on further feedback.

@jonaslagoni
Copy link
Member Author

/progress 50 started working on the implementation and adding tests to ensure it works long term

@jonaslagoni
Copy link
Member Author

Already fixed in #910

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