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

conditionalFiles only works if you are lucky #358

Closed
Tenischev opened this issue May 23, 2020 · 12 comments · Fixed by #363
Closed

conditionalFiles only works if you are lucky #358

Tenischev opened this issue May 23, 2020 · 12 comments · Fixed by #363
Labels
bug Something isn't working released

Comments

@Tenischev
Copy link
Member

Describe the bug

  1. Description of conditionalFiles in docs is incomplete and example is actually pitfall.
  2. If subject is not resolved in AsyncAPI, then file will be rendered because of this condition

How to Reproduce

Just try to use provided example

Expected behavior

  1. Add note that path of file should be relative to tempalte folder
  2. If subject is not found in AsyncAPI, then file is not rendered
  3. Add note that validation is not any JSON Schema Draft 07 object, it must be from JSON Schema Validation
  4. Provide a valid example:
    Correct subject path in term of AsyncAPI spec to server protocols will be servers.*.protocol, but result of this (and any other) resolution will be an array, you should take it into account. Since it's not possible to predict how much servers will be in user AsyncAPI, usage of const is not possible as is. Perhaps, contains should be used always to check against result array.
    Here an example of validation that will correctly check for the mqtt protocol { "contains": { "type": "string", "const": "mqtt" }}.
    Or another approach where filtering delegated to subject - "subject": "servers.*.{protocol: protocol} | [?protocol=='mqtt']", "validation": { "const": [ { "protocol": "mqtt" } ] }

Hint

Perhaps we have a bug in nodejs template because it's exact copy of example from cuurent doc

@Tenischev Tenischev added the bug Something isn't working label May 23, 2020
@derberg derberg added this to the Make Generator stable milestone May 23, 2020
@derberg
Copy link
Member

derberg commented May 26, 2020

One thing sure here is that current conditionalFiles in nodejs-template do not make sense as files listed there do not exist in the tempalate anyway https://github.com/asyncapi/nodejs-template/blob/master/package.json#L88

@derberg
Copy link
Member

derberg commented May 27, 2020

After some investigation:

"conditionalFiles": {
      "config/common.yml": {
        "subject": "server.protocol",
        "validation": {
          "const": "amqp"
        }
      }
    }

and if I generate service using my mqtt server, the file is not generated -> this is what I expected

I'm more concerned about:

@derberg
Copy link
Member

derberg commented May 27, 2020

@Tenischev server.protocol works because this is apparently not a documented feature :)
https://github.com/asyncapi/generator/blob/master/lib/generator.js#L561

@derberg
Copy link
Member

derberg commented May 27, 2020

@Tenischev I tried to replace your hook by configuration in java-template but you do not support server parameter there. If users have 2 servers, how do you guess which one they should use for generation?

@derberg
Copy link
Member

derberg commented May 27, 2020

We could also have a nice warning for template developers in template config validation so they know they have wrong conditions, for files that do not exist. That would be a separate feature issue though

@derberg
Copy link
Member

derberg commented May 27, 2020

@Tenischev I created a PR for java, have a look and tell me what you think https://github.com/asyncapi/java-spring-template/pull/55/files

@derberg
Copy link
Member

derberg commented May 27, 2020

@Tenischev I created PR for docs in generator #363

@fmvilas have a look on my comment

nonRenderableFiles are not supported by conditionalFiles, and I think they should
If you agree I will extend this PR with proper fix

@fmvilas
Copy link
Member

fmvilas commented May 27, 2020

Yeah, I think it totally should 👍

@derberg
Copy link
Member

derberg commented May 28, 2020

@Tenischev
Copy link
Member Author

Hey @derberg ,
Now i see on asyncapi/java-spring-template#55 and want to say "Oh, this is what was mean here in description"
We should expose description of server parameter to describe that you should name one of the servers listed in "servers" of your AsyncAPI
Nevertheless, parameter is very confusing for me, i should think

@derberg
Copy link
Member

derberg commented May 28, 2020

@Tenischev well, the current description of the server is accurate, it just doesn't say anything about conditionalFiles.

I've extended my PR with more explanation, examples and links. What do you think?

@asyncapi-bot
Copy link
Contributor

🎉 This issue has been resolved in version 0.52.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

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

Successfully merging a pull request may close this issue.

4 participants