Skip to content

Fix component spec parsing #1370

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

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

iddeepak
Copy link

Description

This PR resolves the issue described in #1368 by updating the parsing logic in DaprContainer.withComponent(path).

Previously, the type field was incorrectly read from the root level of the component YAML. This caused the type to be null, even though it was correctly defined under the spec section.

The logic has now been fixed to correctly extract type and metadata from within spec.

The corresponding test has also been updated to validate against the expected YAML structure, including type: state.redis and accurate metadata.

Issue reference

Closes #1368

Checklist

  • Code compiles correctly
  • Created/updated tests
  • Extended the documentation (if applicable)

@iddeepak iddeepak requested review from a team as code owners May 15, 2025 16:10
@iddeepak iddeepak force-pushed the fix/component-spec-parsing branch from 90960e1 to eac6d1f Compare May 15, 2025 16:13
Copy link
Contributor

@siri-varma siri-varma left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@iddeepak Thanks a lot for fixing this.

The PR looks good to me.

@artur-ciocanu / @salaboy / @cicoyle Can you folks please help trigger the build for this ?

@salaboy
Copy link
Collaborator

salaboy commented May 15, 2025

workflows running

salaboy
salaboy previously approved these changes May 15, 2025
Copy link
Collaborator

@salaboy salaboy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Copy link
Contributor

@joebowbeer joebowbeer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the metadata parsing is also broken.

Compare the expected result with the original statestore.yaml.

I think the expected metadata should look more like this:

    const expectedComponentYaml =
        "apiVersion: dapr.io/v1alpha1\n"
      + "kind: Component\n"
      + "metadata:\n"
      + "  name: statestore\n"
      + "spec:\n"
      + "  type: state.redis\n"
      + "  version: v1\n"
      + "  metadata:\n"
      + "  - name: keyPrefix\n"
      + "    value: name\n"
      + "  - name: redisHost\n"
      + "    value: redis:6379\n"
      + "  - name: redisPassword\n"
      + "    value: ''\n"

Each Map in the List of Maps should result in one MetadataEntry being added to metadataEntries, but two are actually being added:

      for (Map<String, String> specMetadataItem : specMetadata) {
        for (Map.Entry<String, String> metadataItem : specMetadataItem.entrySet()) {
          metadataEntries.add(new MetadataEntry(metadataItem.getKey(), metadataItem.getValue()));
        }
      }

Should I create a new ticket for that?

@salaboy
Copy link
Collaborator

salaboy commented May 15, 2025

@joebowbeer Yes but also a test .. because it is working as far as I know.

@joebowbeer
Copy link
Contributor

joebowbeer commented May 15, 2025

@salaboy There is a test but I think its expected result is wrong:

Its metadata is very different from the original statestore.yaml:

        + "  metadata:\n"
        + "  - name: name\n"
        + "    value: keyPrefix\n"
        + "  - name: value\n"
        + "    value: name\n"
        + "  - name: name\n"
        + "    value: redisHost\n"
        + "  - name: value\n"
        + "    value: redis:6379\n"
        + "  - name: name\n"
        + "    value: redisPassword\n"
        + "  - name: value\n"
        + "    value: ''\n";

@iddeepak
Copy link
Author

iddeepak commented May 15, 2025

I think the metadata parsing is also broken.

Compare the expected result with the original statestore.yaml.

I think the expected metadata should look more like this:

    const expectedComponentYaml =
        "apiVersion: dapr.io/v1alpha1\n"
      + "kind: Component\n"
      + "metadata:\n"
      + "  name: statestore\n"
      + "spec:\n"
      + "  type: state.redis\n"
      + "  version: v1\n"
      + "  metadata:\n"
      + "  - name: keyPrefix\n"
      + "    value: name\n"
      + "  - name: redisHost\n"
      + "    value: redis:6379\n"
      + "  - name: redisPassword\n"
      + "    value: ''\n"

Should I create a new ticket for that?

Fixed broken metadata and updated changes to same PR now.
@joebowbeer @salaboy

Copy link
Contributor

@joebowbeer joebowbeer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@salaboy This method withComponent(path) has never worked AFAICT because type has always been null, therefore it doesn't surprise me that the parsing of specMetadata was also broken.

@iddeepak iddeepak requested a review from salaboy May 15, 2025 21:28
@joebowbeer
Copy link
Contributor

@iddeepak A style violation is causing a build step to fail?

You have 1 Checkstyle violation.

Signed-off-by: Deepak <[email protected]>
@iddeepak iddeepak force-pushed the fix/component-spec-parsing branch from bb8155a to 35b6588 Compare May 16, 2025 12:07
Copy link

codecov bot commented May 16, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 77.49%. Comparing base (d759c53) to head (35b6588).
Report is 145 commits behind head on master.

Additional details and impacted files
@@             Coverage Diff              @@
##             master    #1370      +/-   ##
============================================
+ Coverage     76.91%   77.49%   +0.58%     
- Complexity     1592     1772     +180     
============================================
  Files           145      204      +59     
  Lines          4843     5444     +601     
  Branches        562      597      +35     
============================================
+ Hits           3725     4219     +494     
- Misses          821      905      +84     
- Partials        297      320      +23     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

DaprContainer withComponent(path) incorrectly parses the type field
4 participants