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

Model representation in the form of Node in AST #628

Closed
magicmatatjahu opened this issue Feb 10, 2022 · 12 comments
Closed

Model representation in the form of Node in AST #628

magicmatatjahu opened this issue Feb 10, 2022 · 12 comments
Labels
enhancement New feature or request keep Dont let stale get to it. stale

Comments

@magicmatatjahu
Copy link
Member

magicmatatjahu commented Feb 10, 2022

As I'm the "author" of the presets, I see that I made a big mistake of treating them as middlewares which are supposed to return the changed content (as a string) and not as data (in meaning of the structure) that is later changed to the correct model in the given language (class/interface/structure etc).

It seems to me that we should move to representing a given model as a node in a typical AST. What does that mean? During the processing of presets, the user/developer would have access to the model representation as a structure (not a string!) and then, after each preset (btw. we should change the name to hook), this structure should be transformed into a valid model in the given language. Here is an example to better understand the idea:

interface JavaClassNode {
  name: string;
  modifiers: Array<'public', 'private', 'static' ...>;
  annotations: Record<string, {
    ...
  }>;
  interfaces: Array<Type>; // interfaces which implements given class
  extends: Type; // we can extend only by one class
  properties: Record<string, {
    modifiers: Array<'public', 'private', 'final', 'protected', ...>;
    type: Type,
    initialValue: string,
    ...
  }>;
  methods: Record<string, {
    modifiers: Array<'public', 'private', 'final', 'protected', ...>;
    arguments: Array<{ name: string, type: Type }>;
    returnType: Type;
    body: string; // I think that body of method still be of string type, but we can discuss about it
  }>;
  ... // other things like comments etc
}

then we will have hooks only for given type kind - in Java class, interface, enum (maybe record?) - and we can remove all presets for properties, additionalProperties etc, because it's bullshit and they are difficult to use. Inside hooks we will be able to change that internal data in structure (and what is important, add/remove/update new properties, methods, annotations etc), e.g.:

// java hooks
const hook1 = {
  class: ({ model }) => {
    model.modifiers.push('private');
    model.interfaces.push({ name: 'ISomeInterface' });
    model.annotations.push({ name: 'SomeAnnotation', value: ... });
    model.extends = { name: 'AnotherJavaClass' };
  },
  interface: () => {...},
  enum: () => {...},
}
const hook2 = {
  class: ({ model }) => {
    model.interfaces.push({ name: 'IAnotherInterface' });
    model.properties['someProperty'] = { modifiers: ['private'], type: { name: 'SomeType', initialValue = 'new SomeType()' } };
    model.methods['someMethod'] = { modifiers: ['protected'], returnType: { name: 'SomeReturnType' }, arguments: [{ name: 'arg1', type: { name: 'SomeType' } }] };
  },
  interface: () => {...},
  enum: () => {...},
}

and then after running that hooks we will have rendered model:

@SomeAnnotation(...)
private class JavaClass extends AnotherJavaClass implements ISomeInterface, IAnotherInterface {
  private SomeType someProperty = new SomeType();
  protected SomeReturnType someMethod(SomeType arg1, ...) {...}
}

Benefits:

  • we don't operate on string and perform some regex to "retrieve" some metadata defined by previous defined presets, but we operate on the data structure, so we can easily add/remove/update given property/method/annotations (depending on the language) etc.
  • the representation in one language can be converted into another language representation (easier integration - that current one - of solutions like TS input data etc), because we will operate on AST.
  • since no one will be operating on strings (only for method/function body), this eliminates the problem of a given preset being incompatible with another and creating a bad syntax for given language.

Minuses:

  • we still need to operate on string for method/function body. I don't know if we should switch to the AST in this case, WDYT?
  • we still have boilerplate to add "metadata" for model, but I think that it's still much less that current solution with string operations.
  • each language and model type will have a different AST, resulting in a large number of interfaces on the core side - but that's not a problem for me -> see 2nd benefit point.

What do you think? I don't think we'll ever find a best solution that's easy to use and powerful, but this one is much easier than the current string-based presets.

cc @jonaslagoni @smoya
I see that you @panwauu @mahakporwal02 @ron-debajyoti are very active in that project. I think that it is interesting for you :)

@magicmatatjahu
Copy link
Member Author

interesting tool for AST processing https://github.com/antlr/antlr4

@jonaslagoni
Copy link
Member

Good proposal, thanks for taking the time @magicmatatjahu. I would suggest we try to dial down into how this will affect the UX of presets through the following use-cases (feel free to add more).

Let's keep it in TypeScript:

  1. Modifying property visibility (from private to public) and removing getter and setters.
  2. Adding a new function per property
  3. Modifying the getter and setter functions for a class
  4. Adding a custom function at the end of the class

Do you want to make the examples both for AST and the current approach, or do you want me to take the string middlewares?

@magicmatatjahu
Copy link
Member Author

magicmatatjahu commented Feb 11, 2022

@jonaslagoni I can handle some examples:

  • Modifying property visibility (from private to public) and removing getter and setters.
const hook = {
  class: ({ model }) => {
    model.modifiers.push('public'); // we have to know that in TS you can have one 'public' | 'private' | 'protected'. Then we can render the last defined (or first defined) or create helpers function to override previously defined `private` etc modifiers
    model.setters.remove('someProperty');
    model.getters.remove('someProperty');
  },
}
  • Adding a new function per property
const hook = {
  class: ({ model }) => {
    model.methods['someFunction'] = {
      arguments: [{ name: 'someArg', type: { name: 'SomeType' }, optional: true }],
      returnType: { name: 'ReturnType' },
      modifiers: ['protected'],
      body: ...
    }
  }
}

// rendered class
class SomeClass {
  protected someFunction(someArg?: SomeType): ReturnType {
    // body
  }
}
  • Modifying the getter and setter functions for a class
const hook = {
  class: ({ model }) => {
    if (model.getters['someProperty']) { // check if given getter exist
        model.getters['someProperty'] = {
        ...model.getters['someProperty'],
        // update fields
      }
    }
  }
}
  • Adding a custom function at the end of the class
const hook = {
  class: ({ model }) => {
    model.methods['someFunction'] = {
      ...
    }
  }
}

You can add your own or write what you don't like in my examples - maybe you see some inaccuracies in my examples.

Of course you can handle that examples in the current approach :)

@jonaslagoni
Copy link
Member

You can add your own or write what you don't like in my examples - maybe you see some inaccuracies in my examples.

Remember, in the hooks, you generally don't know anything about what properties may be there. So try with the abstraction of only knowing you have the internal model with a map of properties 🙂

Also, the body is important to clarify, because would you need to build the body of the function with AST as well? Or how would you do that?

@magicmatatjahu
Copy link
Member Author

Remember, in the hooks, you generally don't know anything about what properties may be there. So try with the abstraction of only knowing you have the internal model with a map of properties 🙂

I'm giving examples of how it could look like for a model in TS, I think that if someone understands the idea and knows another language, they know how it could look in another language :)

Also, the body is important to clarify, because would you need to build the body of the function with AST as well? Or how would you do that?

I have that problem and I raise it in the one of minuses given solution:

we still need to operate on string for method/function body. I don't know if we should switch to the AST in this case, WDYT?

The problem is that here we are getting slowly into the so-called expression nodes, i.e. loops, assignments, mathematical evaluations etc... and there is a lot of it in Java (as well as in other languages). It seems to me that operating on string is much easier in this case. Fields and constructor are the most important.

@magicmatatjahu
Copy link
Member Author

Edit: There is always an option to add such nodes (I mean nodes for the function/method body) successively, but the body of the function/method should then be either a string or a AST node :)

@felixjung
Copy link

Ran into this problem today, seeing there was a PR with a preset for JSON tags in Go and my PR adding a preset to render descriptions as comments -- how do you get both JSON tags on a struct and the comments?

I also thought that an AST being passed through a set of modifier functions would be the solution.

I'm somewhat afraid of a huge effort to have ASTs in TypeScript for all the languages you want to support. Typically the languages targeted by Modelina will have pretty good support for their own AST, at least that's the case for Go.

Additionally, I was already somewhat hesitant to use a JS library to generate code for my Go codebase. Potentially, it may be easier to have language-specific tooling written in the target language. You would then be able to nicely run language-specific tooling as part of the regular build process. In Go there is the generate command that can be triggered from special comments inside the codebase.

With this approach you don't need as many abstractions as long as there is a parser for AsyncAPI. But maybe I'm underestimating the effort to create a parser in every language. 😅

@magicmatatjahu
Copy link
Member Author

@felixjung Thanks for that comment!

Just because a language has AST support does not mean that we should generate models for that language (e.g. Go) in the same language (Go).

The problem is (as you noted) the very existence of the parser. Even though we currently have a parser written in go, it still has (maybe I'm exaggerating) 1/4 of the capabilities of the current parser written in JS. Take into account that AsyncAPI has the ability to define schemas (the structure of which is used to generate models) not only in JSON Schema, but also in Avro, Raml, OpenaAPI JSON Schema and in the future also in Protobuf or XSD, and the go parser doesn't support it at all and probably won't support it in the coming year because nobody has the time to contribute to it - I don't even want to point out that AsyncAPI has defined custom validation conditions that are not possible with JSON Schema itself and ParserJS supports (not all) such conditions then ParserGO does not support it at all. Taking into account other languages we still have the same problem. I do not think that anyone will ever write a parser in (e.g.) Dart, but the models themselves would like to be generated (e.g. for a client). Modelina itself is not only used to generate models in single projects, but it can also be used to create models in templates for our generator (which is written in JS and the template itself must be written in JS to be generated) and we also want to run it in a browser, so we're stuck with JS.

So the only advantage of writing models in a given language is good AST support, but nothing else.

Of course, there's nothing stopping you from using a Modelina and then making some GoLang code (based on some JSON produces by Modelina, tbh I don't know), but I don't think we'd officially support that, because we prioritize broad language support without weird "extensions" for those languages.

@github-actions
Copy link
Contributor

This issue has been automatically marked as stale because it has not had recent activity 😴

It will be closed in 120 days if no further activity occurs. To unstale this issue, add a comment with a detailed explanation.

There can be many reasons why some specific issue has no activity. The most probable cause is lack of time, not lack of interest. AsyncAPI Initiative is a Linux Foundation project not owned by a single for-profit company. It is a community-driven initiative ruled under open governance model.

Let us figure out together how to push this issue forward. Connect with us through one of many communication channels we established here.

Thank you for your patience ❤️

@github-actions github-actions bot added the stale label Jun 18, 2022
@github-actions github-actions bot removed the stale label Jul 29, 2022
@github-actions
Copy link
Contributor

This issue has been automatically marked as stale because it has not had recent activity 😴

It will be closed in 120 days if no further activity occurs. To unstale this issue, add a comment with a detailed explanation.

There can be many reasons why some specific issue has no activity. The most probable cause is lack of time, not lack of interest. AsyncAPI Initiative is a Linux Foundation project not owned by a single for-profit company. It is a community-driven initiative ruled under open governance model.

Let us figure out together how to push this issue forward. Connect with us through one of many communication channels we established here.

Thank you for your patience ❤️

@github-actions
Copy link
Contributor

This issue has been automatically marked as stale because it has not had recent activity 😴

It will be closed in 120 days if no further activity occurs. To unstale this issue, add a comment with a detailed explanation.

There can be many reasons why some specific issue has no activity. The most probable cause is lack of time, not lack of interest. AsyncAPI Initiative is a Linux Foundation project not owned by a single for-profit company. It is a community-driven initiative ruled under open governance model.

Let us figure out together how to push this issue forward. Connect with us through one of many communication channels we established here.

Thank you for your patience ❤️

@github-actions github-actions bot added the stale label Jun 14, 2023
@jonaslagoni jonaslagoni added keep Dont let stale get to it. and removed stale labels Jun 14, 2023
Copy link
Contributor

This issue has been automatically marked as stale because it has not had recent activity 😴

It will be closed in 120 days if no further activity occurs. To unstale this issue, add a comment with a detailed explanation.

There can be many reasons why some specific issue has no activity. The most probable cause is lack of time, not lack of interest. AsyncAPI Initiative is a Linux Foundation project not owned by a single for-profit company. It is a community-driven initiative ruled under open governance model.

Let us figure out together how to push this issue forward. Connect with us through one of many communication channels we established here.

Thank you for your patience ❤️

@github-actions github-actions bot added the stale label Jan 12, 2024
@github-actions github-actions bot closed this as not planned Won't fix, can't repro, duplicate, stale May 13, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request keep Dont let stale get to it. stale
Projects
None yet
Development

No branches or pull requests

3 participants