modelina

modelina copied to clipboard

Description

Add Avro Schema support.

Related Issue

fixes #1741

Checklist

• [x] The code follows the project's coding standards and is properly linted (npm run lint).
• [x] Tests have been added or updated to cover the changes.
• [x] All tests pass successfully locally.(npm run test).
• [ ] Documentation has been updated to reflect the changes.

✅ Deploy Preview for modelina canceled.

Name	Link
🔨 Latest commit	ef792c66189c00e1a1d28e80f8c78aedfd014a0f
🔍 Latest deploy log	https://app.netlify.com/sites/modelina/deploys/662b6363d172e50008a01f9a

hey @akkshitgupta how is it coming along? Are you stuck on anything? :v:

since it is required to obtain the meta model directly from the Avro Schema bypassing the common model. I am trying to figure out how a schema is using common model in between to obtain the meta model. I read the different meta models but stuck at the implementation of it.

What are you currently stuck on? Getting started with converting them or a specific example you cant convert?

Getting started with it. I was reading the code base and connecting some of the dots for reverse engineering to find the way backward from meta model. but unfortunately could not use the meta models. perhaps failing to understand the flow for generating the meta model.

PS: I selected this issue to work on so that I can understand the working of Common Model and Meta Model in a different way.

I will much obliged to you if you can share some insights either here or on Slack(for clean history)

@akkshitgupta looks like you got figured out some of it, let me know if you have anything blocking you 😄

Hey @jonaslagoni I forgot to ping you in this particular comment. can you please go through this once. I am trying to test the processor but stuck at the bindings.

Also wanna confirm that whether we are going with the same .json file extension or gonna adopt the .avsc file extension and add a support for that as well. :)

What those bindings are I have no idea 🤨 Are you using node v18?

Hey @akkshitgupta, have you had a chance to take a look at the review comments?

Apologies for the delay @jonaslagoni I am working on it. some tests broke yesterday figuring out what happened.

Quality Gate failed

Failed conditions
8.4% Duplication on New Code (required ≤ 3%)

See analysis details on SonarCloud

Hey @jonaslagoni, I am stuck at the process of converting an array as I could not figure out the keyword which would represent the entire array.

here the default keyword is used to represent an empty array.

@akkshitgupta in AsyncAPI spec-json-schema repo we have this JSON Schema document that can validates Avro documents: https://github.com/asyncapi/spec-json-schemas/blob/master/common/avroSchema_v1.json

I suggest we focus on that, because yea, the spec is not clear on what items: the schema of the array’s items. can contain.

According to the JSON Schema: https://github.com/asyncapi/spec-json-schemas/blob/35ea79055d09bdd319ed816e4ec169a12f0bb7a5/common/avroSchema_v1.json#L225 it can contain types, which is: https://github.com/asyncapi/spec-json-schemas/blob/35ea79055d09bdd319ed816e4ec169a12f0bb7a5/common/avroSchema_v1.json#L14

here the default keyword is used to represent an empty array.

Default is just hmmm, yea I see what you mean, what is default 🤔 I actually think you can ignore it :v:

I have some doubts regarding spec-json-schema

• are we gonna implement spec-json-schema validation instead of Avro Schema defined here or we just need to implement the scenario/concept used there for avro array and use that implementation while getting the Array Meta Model
• not understood this particular thing, what you are referring to in here

  I suggest we focus on that

Note: I have some academic exams and I might be late in replying during this coming week. Apologies for that.

I have some doubts regarding spec-json-schema

* are we gonna implement `spec-json-schema` validation instead of [Avro Schema](https://github.com/asyncapi/modelina/blob/73f47d7f7a326cb7f40e71658574c3b9e9510f71/src/models/AvroSchema.ts) defined here or we just need to implement the scenario/concept used there for `avro array` and use that implementation while getting the Array Meta Model

* not understood this particular thing, what you are referring to in here
  > I suggest we focus on that

Note: I have some academic exams and I might be late in replying during this coming week. Apologies for that.

The only reason why I linked the spec-json-schema files, was to show the structure of how an array is defined.

But I guess we could validate the input as well 🤔 I would forget about it in this PR though.

No worries, no rush :v: Good luck in the exams!

Hey @jonaslagoni, I am getting an error whenever I run the integration test using test/processors/AvroSchemaInputProcessor.spec.ts.

However, the unit tests are running fine and there is no typeerror. I tried to google the solution but could not find success. can you help me with this particular thing 😄

@akkshitgupta what happens when you try running includes on object type such as here: https://github.com/asyncapi/modelina/pull/1753/files#diff-92b54f6485623708e919c7944aa95ef0c21d22401eeb4d2ee3c35cdafd57eb1eR24 😄

Pull Request Test Coverage Report for Build 8832734915

Details

• 114 of 153 (74.51%) changed or added relevant lines in 7 files are covered.
• No unchanged relevant lines lost coverage.
• Overall coverage decreased (-0.5%) to 91.783%

---

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
src/helpers/AvroToMetaModel.ts	88	127	69.29%
<!--	Total:	114	153

Totals
Change from base Build 8831670254:	-0.5%
Covered Lines:	6109
Relevant Lines:	6479

---

💛 - Coveralls

Hello, @coveralls! 👋🏼

    I'm 🧞🧞🧞 Genie 🧞🧞🧞 from the magic lamp. Looks like somebody needs a hand!

    At the moment the following comments are supported in pull requests:

    - `/please-take-a-look` or `/ptal` - This comment will add a comment to the PR asking for attention from the reviewrs who have not reviewed the PR yet.
    - `/ready-to-merge` or `/rtm` - This comment will trigger automerge of PR in case all required checks are green, approvals in place and do-not-merge label is not added
    - `/do-not-merge` or `/dnm` - This comment will block automerging even if all conditions are met and ready-to-merge label is added
    - `/autoupdate` or `/au` - This comment will add `autoupdate` label to the PR and keeps your PR up-to-date to the target branch's future changes. Unless there is a merge conflict or it is a draft PR.

@akkshitgupta remember to resolve review comments when you have addressed them one way or another, and just ping me when you need another review :v:

@jonaslagoni can you have a look at the confusion I mentioned above 🙏🏻

@akkshitgupta I cannot see your comment

@jonaslagoni these two

https://github.com/asyncapi/modelina/pull/1753#discussion_r1564241626 https://github.com/asyncapi/modelina/pull/1753#discussion_r1564548748

@akkshitgupta for some reason I cannot see them at all 🤨

Do you mind posting them again or as a comment? 🤔

Here are the comment images @jonaslagoni 😅

@akkshitgupta do you see the pending label? It means that you wrote those comments as part of a review that you have not published. You have to add them as single comments 🙂

@jonaslagoni extremely sorry for the confusion 🙇🏻🙇🏻

I thought these comments are the part of unresolved conversation thats why it is showing pending 😓

@akkshitgupta is it something you want to include in this PR, or want to merge this one and add issues to describe what is missing?

hello @jonaslagoni I have not been active for the past few days for some reason and also have my end-of-semester exams in the coming week. It would be great to merge this PR with other PRs to mention related issues, if that makes sense 😅😅. However, I would make sure to complete this ASAP. Thanks a lot for your kind support in my journey 😄

@akkshitgupta do you mind listing all the current short comings of the implementation right now?