generator icon indicating copy to clipboard operation
generator copied to clipboard

fix: correctly detect default values including false in template parameters

Open lightning-sagar opened this issue 3 weeks ago • 9 comments

Description

This PR fixes an issue where template parameters with a default value of false (e.g., singleFile) were not being applied, causing them to appear as undefined inside templates.

The problem was caused by this check:

 const defaultValues = Object.keys(parameters || {}).filter(key => parameters[key].default);

Since false is falsy, it was incorrectly ignored. The fix updates the condition to properly detect all default values:

const defaultValues = Object.keys(parameters || {}).filter(key => parameters[key].default !== undefined);

How I verified the fix


Related issue(s)

Fixes https://github.com/asyncapi/generator/issues/1768

Summary by CodeRabbit

  • Bug Fixes

    • Default value detection now correctly treats falsy defaults (0, false, "") as defined so they are applied reliably.
  • New Features

    • Added per-schema compilation and single-schema validation utilities for message schema workflows, and adjusted validation flows to return structured results.
  • Documentation

    • Updated contributor and onboarding guidance.
  • Chores

    • Consolidated shared test configuration and moved local test settings into directory configs.
  • Tests

    • Expanded and adapted tests to cover new compile/validate behaviors and error cases.

✏️ Tip: You can customize this high-level summary in your review settings.

lightning-sagar avatar Nov 25 '25 13:11 lightning-sagar

⚠️ No Changeset found

Latest commit: 802b021180cd16d5f64a1cbb628a447906761217

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

changeset-bot[bot] avatar Nov 25 '25 13:11 changeset-bot[bot]

What reviewer looks at during PR review

The following are ideal points maintainers look for during review. Reviewing these points yourself beforehand can help streamline the review process and reduce time to merge.

  1. PR Title: Use a concise title that follows our Conventional Commits guidelines and clearly summarizes the change using imperative mood (it means spoken or written as if giving a command or instruction, like "add new helper for listing operations")

    Note - In Generator, prepend feat: or fix: in PR title only when PATCH/MINOR release must be triggered.

  2. PR Description: Clearly explain the issue being solved, summarize the changes made, and mention the related issue.

    Note - In Generator, we use Maintainers Work board to track progress. Ensure the PR Description includes Resolves #<issue-number> or Fixes #<issue-number> this will automatically close the linked issue when the PR is merged and helps automate the maintainers workflow.

  3. Documentation: Update the relevant Generator documentation to accurately reflect the changes introduced in the PR, ensuring users and contributors have up-to-date guidance.

  4. Comments and JSDoc: Write clear and consistent JSDoc comments for functions, including parameter types, return values, and error conditions, so others can easily understand and use the code.

  5. DRY Code: Ensure the code follows the Don't Repeat Yourself principle. Look out for duplicate logic that can be reused.

  6. Test Coverage: Ensure the new code is well-tested with meaningful test cases that pass consistently and cover all relevant edge cases.

  7. Commit History: Contributors should avoid force-pushing as much as possible. It makes it harder to track incremental changes and review the latest updates.

  8. Template Design Principles Alignment: While reviewing template-related changes in the packages/ directory, ensure they align with the Assumptions and Principles. If any principle feels outdated or no longer applicable, start a discussion these principles are meant to evolve with the project.

  9. Reduce Scope When Needed: If an issue or PR feels too large or complex, consider splitting it and creating follow-up issues. Smaller, focused PRs are easier to review and merge.

  10. Bot Comments: As reviewers, check that contributors have appropriately addressed comments or suggestions made by automated bots. If there are bot comments the reviewer disagrees with, react to them or mark them as resolved, so the review history remains clear and accurate.

asyncapi-bot avatar Nov 25 '25 13:11 asyncapi-bot

Walkthrough

Changed default-detection in the template config loader: parameters are now treated as having a declared default if they explicitly include a default property (including falsy values like false, 0, or "") by using Object.hasOwn instead of a truthy check.

Changes

Cohort / File(s) Summary
Default detection change
apps/generator/lib/templates/config/loader.js
Replace truthy check on parameters[key].default with Object.hasOwn(parameters[key], 'default') to detect declared defaults including falsy values; no other logic or control flow altered.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

  • Verify the Object.hasOwn usage (compatibility / polyfill concerns) and that it targets only the intended object shapes.
  • Check or add tests for falsy defaults: false, 0, "".
  • Confirm lazy-default getters still behave as expected in downstream consumers.

Possibly related PRs

  • asyncapi/generator#1485 — changes to template configuration loading flow and loadDefaultValues, directly related to default-detection logic.
  • asyncapi/generator#1594 — introduces/relies on .ageneratorrc and template config handling that interacts with default-loading behavior.
  • asyncapi/generator#1747 — integrates the keeper package and schema compile/validate features that may consume template defaults in generated templates.

Pre-merge checks and finishing touches

❌ Failed checks (2 warnings)
Check name Status Explanation Resolution
Out of Scope Changes check ⚠️ Warning While the primary fix targets template parameter defaults (apps/generator/lib/templates/config/loader.js), the PR includes extensive unrelated changes to workflows, documentation, Jest configuration, and keeper package refactoring that fall outside the stated objective. Separate out-of-scope changes (workflows, documentation, keeper refactoring, Jest configs) into distinct PRs focused on the template parameter fix and test coverage.
Docstring Coverage ⚠️ Warning Docstring coverage is 66.67% which is insufficient. The required threshold is 80.00%. You can run @coderabbitai generate docstrings to improve docstring coverage.
✅ Passed checks (3 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title accurately and clearly summarizes the main change: fixing detection of falsy default values (specifically false) in template parameters.
Linked Issues check ✅ Passed The PR addresses the core requirement from issue #1768: correctly detecting and injecting default values set to false into template context, enabling conditional logic to work properly.
✨ Finishing touches
  • [ ] 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • [ ] Create PR with unit tests
  • [ ] Post copyable unit tests in a comment

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

coderabbitai[bot] avatar Nov 25 '25 13:11 coderabbitai[bot]

Note: After fixing this and changing the value from the string "false" to the boolean false, the CSS/JS will generate correctly🫡

By using asyncapi generate fromTemplate test/spec/asyncapi_v3.yml ./ -o dupa --use-new-generator --force-write -p singleFile="false" this command not by -p singleFile=false

lightning-sagar avatar Nov 26 '25 16:11 lightning-sagar

@lightning-sagar I reviewed the changes you made. Using: filter(key => parameters[key].default !== undefined); which i say is not bad does fix the false issue, but it may introduce new problems. For example, templates can intentionally set: "default": null or even: "default": undefined In those cases, !== undefined would incorrectly treat these values as valid defaults. to Avoid that problem @derberg Solution to use const defaultValues = Object.keys(parameters || {}).filter(key => hasOwnProperty('default')); but it also have a major issue This checks whether the template author actually defined the default field, regardless of its value. It avoids accidentally ignoring defaults like false, 0, or "".

We avoid using obj.hasOwnProperty() directly because it can throw errors if the object does not inherit from Object.prototype. So to Avoid crashing and error we can use explicit version of js something like const defaultValues = Object.keys(parameters || {}).filter(key => Object.prototype.hasOwnProperty.call(parameters[key], "default") ); this guarantees no crashes and works with all template config objects.

https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Object/hasOwnProperty image

Jatin24062005 avatar Dec 05 '25 14:12 Jatin24062005

yes, that makes sense. your solution is definitely more robust. using Object.prototype.hasOwnProperty.call(...) handles both JSON and JS-generated templates safely, and avoids any issues with falsy defaults or prototype-less objects. thanks for the help!

update: I switched to Object.hasOwn(...) initially since it reads cleaner.

lightning-sagar avatar Dec 06 '25 21:12 lightning-sagar

@derberg I noticed coderabbit flagged a bunch of issues in files I didnt touch (Keeper, WebSocket client templates, Constructor.js, etc) looks like these came in from the recent merge into master, not from the changes in this pr

before I do anything, could you let me know what you'd prefer?

  • should I rebase/merge my branch with the latest master and fix those warnings here or

  • should this pr stay focused only on the default-parameter fix, and the other warnings be handled separately?

just want to keep the scope clean and avoid mixing unrelated fixes

lightning-sagar avatar Dec 07 '25 08:12 lightning-sagar

@derberg please review whenever you have time

lightning-sagar avatar Dec 13 '25 19:12 lightning-sagar