refactor: move out the loadTemplateConfig from generator.js
#1584
Conversation
Signed-off-by: ItsRoy69 <jyotirmoyroy649@gmail.com>
ItsRoy69
requested review from
derberg,
magicmatatjahu,
jonaslagoni and
asyncapi-bot-eve
as code owners
|
|
""" WalkthroughThe changes refactor the template configuration loading logic in the generator by removing internal methods from the Changes
Assessment against linked issues
Assessment against linked issues: Out-of-scope changesNo out-of-scope changes identified. Possibly related PRs
📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
🔇 Additional comments (6)
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Welcome to AsyncAPI. Thanks a lot for creating your first pull request. Please check out our contributors guide 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.
|
Hey @ItsRoy69 Have you run the test by |
|
@derberg can you please start the workflow? |
Signed-off-by: ItsRoy69 <jyotirmoyroy649@gmail.com>
|
@ItsRoy69 go through these issues. |
|
here are some of the issues. |
Signed-off-by: ItsRoy69 <jyotirmoyroy649@gmail.com>
|
@ItshMoh hey, updated the path accordingly and tested out , its passing now. |
|
@ItsRoy69 There are also some issues in dart client. fix it. The snapshots are obselete, generate new ones. |
|
@ItsRoy69 also run |
Signed-off-by: ItsRoy69 <jyotirmoyroy649@gmail.com>
sure, will go through it , noticed many testings are failing in other files, looking into it |
|
@ItshMoh the integration issue is separate issue, I guess that can be handled in separate PR as its quite many changes required for that which is not related to template generator error |
@ItsRoy69 the integration issue is not the separate issue. it is due to the changes we made in this PR. So we need to fix it here. Otherwise how is it gonna be merged?. Look for it |
|
@ItsRoy69 modify the |
Signed-off-by: ItsRoy69 <jyotirmoyroy649@gmail.com>
…into fix-template
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
apps/generator/lib/templateConfig/loader.js (3)
7-10: Fix JSDoc annotation inconsistency.The function is marked as
@privatebut is exported in the module. Consider either removing the@privateannotation or adding appropriate public documentation./** * Loads the template configuration. - * @private + * @param {Object} context - The generator instance context (bound via .call()) */
41-45: Fix JSDoc annotation inconsistency.Same issue as the
loadTemplateConfigfunction - marked as@privatebut exported./** * Loads default values of parameters from template config. If value was already set as parameter it will not be - * overriden. - * @private + * overridden. + * @param {Object} context - The generator instance context (bound via .call()) */Note: Also fixed the typo "overriden" → "overridden".
46-58: Consider the context dependency design.The functions rely heavily on
thiscontext (accessingthis.templateDir,this.templateConfig,this.templateParams), which requires careful binding when called. While this maintains compatibility with the existing codebase during refactoring, consider whether a more explicit parameter-passing approach might be more maintainable long-term.For now, the current approach is acceptable given the refactoring context.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
apps/generator/lib/generator.js(3 hunks)apps/generator/lib/templateConfig/loader.js(1 hunks)apps/generator/lib/templateConfig/validator.js(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- apps/generator/lib/templateConfig/validator.js
- apps/generator/lib/generator.js
🔇 Additional comments (1)
apps/generator/lib/templateConfig/loader.js (1)
11-39: Excellent refactoring approach with proper error handling.The configuration loading logic is well-structured with appropriate fallback mechanisms:
- Primary attempt:
.ageneratorrc(YAML format)- Fallback:
package.jsonwithgeneratorproperty- Graceful error handling with debug logging
The refactoring successfully extracts this logic from the main generator file while maintaining the same behavior.
Signed-off-by: ItsRoy69 <jyotirmoyroy649@gmail.com>
Signed-off-by: ItsRoy69 <jyotirmoyroy649@gmail.com>
|
@derberg can you run the workflow when you are free, and also if you can review the decoupling part. |
derberg
changed the title
loadTemplateConfig from generator.js
There was a problem hiding this comment.
I'm concerned about loadDefaultValues.call(this) approach
even though it is possible, we don't do it across the whole project. We follow functions composition as it is better for readability
simply better to have loadDefaultValues that accepts an argument templateConfig that are well documented with jsdoc, and it also returns whatever is needed.
this is cool but when really directly only used inside one a class, within the same file
same with loadDefaultValues
can you please refactor?
|
Yes, working on it |
Signed-off-by: ItsRoy69 <jyotirmoyroy649@gmail.com>
Signed-off-by: ItsRoy69 <jyotirmoyroy649@gmail.com>
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
apps/generator/lib/templateConfig/loader.js (1)
19-19:⚠️ Potential issueSecurity vulnerability remains unfixed despite previous review
The YAML parsing still uses the unsafe
load()method which can execute arbitrary code. This was flagged in a previous review but appears to remain unaddressed despite being marked as resolved.Apply this fix immediately:
- const yamlConfig = require('js-yaml').load(yaml); + const yamlConfig = require('js-yaml').load(yaml, { schema: require('js-yaml').JSON_SCHEMA });
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
apps/generator/lib/generator.js(2 hunks)apps/generator/lib/templateConfig/loader.js(1 hunks)apps/generator/test/generator.test.js(2 hunks)
✅ Files skipped from review due to trivial changes (1)
- apps/generator/test/generator.test.js
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/generator/lib/generator.js
🔇 Additional comments (3)
apps/generator/lib/templateConfig/loader.js (3)
1-5: LGTM: Clean module setupThe imports and constants are well-organized and appropriate for the module's functionality.
49-63: LGTM: Elegant default value implementationThe use of
Object.definePropertywith getters provides lazy evaluation of default values, which is an efficient approach. The filtering logic correctly handles undefined parameters.
65-68: LGTM: Clean module exportsThe module exports are well-structured and expose the necessary functions.
Signed-off-by: ItsRoy69 <jyotirmoyroy649@gmail.com>
Signed-off-by: ItsRoy69 <jyotirmoyroy649@gmail.com>
Signed-off-by: ItsRoy69 <jyotirmoyroy649@gmail.com>
|
@ItshMoh refactored the code accordingly, tested npm run lint, npm run test and npm run lint:fix as well |
| } | ||
| }) | ||
| ); | ||
| loadDefaultValues() { |
There was a problem hiding this comment.
why we need this here, also implementation of loadTemplateConfig() if anyway they are both private? what is blocking us from calling loadDefaultValues inside loadTemplateConfig directly in loader.js?
There was a problem hiding this comment.
Added loadTemplateConfig.bind(this) and loadDefaultValues.bind(this) to bind the imported function to the class instance instead of implementation of loadTemplateConfig() and loadTemplateConfig() in generator.js
There was a problem hiding this comment.
but why did you do it, call or bind add weird complexity to code
we discussed before that sharing this instead of having arguments in functions is causing complexity
Signed-off-by: ItsRoy69 <jyotirmoyroy649@gmail.com>
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
apps/generator/lib/templateConfig/loader.js (1)
11-41: Verify security fix implementation and add robust input validation.Despite past review comments indicating these issues were addressed, the current code still shows potential problems:
- Security: Line 18 still uses
js-yaml.load()which can execute arbitrary code- Input validation: No validation for
this.templateDirexistence or typePlease verify that the security fixes were properly applied:
#!/bin/bash # Check if js-yaml is being used safely rg -A 2 -B 2 "js-yaml.*load" apps/generator/lib/templateConfig/loader.js # Check for any input validation in the function rg -A 5 -B 2 "templateDir.*string\|typeof.*templateDir" apps/generator/lib/templateConfig/loader.js
🧹 Nitpick comments (1)
apps/generator/lib/templateConfig/loader.js (1)
48-60: Consider defensive programming for parameter handling.The function assumes
this.templateConfig.parametersandthis.templateParamsexist. While this may be guaranteed by the calling context, adding defensive checks would improve robustness.Consider adding safety checks:
async function loadDefaultValues() { + if (!this.templateConfig || !this.templateParams) { + log.debug('Missing templateConfig or templateParams, skipping default values'); + return; + } + const parameters = this.templateConfig.parameters; const defaultValues = Object.keys(parameters || {}).filter(key => parameters[key].default);
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
apps/generator/lib/generator.js(3 hunks)apps/generator/lib/templateConfig/loader.js(1 hunks)apps/generator/test/generator.test.js(2 hunks)
✅ Files skipped from review due to trivial changes (1)
- apps/generator/test/generator.test.js
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/generator/lib/generator.js
🔇 Additional comments (1)
apps/generator/lib/templateConfig/loader.js (1)
62-65: LGTM: Clean module exports.The module exports are well-structured and follow Node.js conventions.
Signed-off-by: ItsRoy69 <jyotirmoyroy649@gmail.com>
|
hey @Adi-204 Can you please review this PR? It would be a great help. |
|
There was a problem hiding this comment.
the biggest problem here is that the PR is about refactoring, moving code from one place to another - but I see there is some logic added, some sync functions that are not in scope.
also, why even though loadTemplateConfig and loadDefaultValues were moved, they are still having some implementations in the generator.js and are not just used directly there without these implementations
like for example below
loadTemplateConfig() {
return loadTemplateConfig(this.templateDir).then(config => {
this.templateConfig = config;
return this.loadDefaultValues();
});
}
this is completely unnecessary
just line 289 await this.loadTemplateConfig(); must be modified, so new function is used, and proper arguments are passed, and this.templateConfig gets assigned whatever loadTemplateConfig returns
and loadDefaultValues should not even be mentioned once in generator.js but should be used internally inside loader.js
Description
Related issue(s)
Resolves #1582
Summary by CodeRabbit
Refactor
Chores