Skip to content

Refactor role claim parsing for issue #595#601

Merged
ziagham merged 1 commit intoflowsynx:masterfrom
mohin-io:fix/reduce-addroles-complexity-595
Oct 29, 2025
Merged

Refactor role claim parsing for issue #595#601
ziagham merged 1 commit intoflowsynx:masterfrom
mohin-io:fix/reduce-addroles-complexity-595

Conversation

@mohin-io
Copy link
Copy Markdown
Contributor

Summary

  • Refactored RoleClaimsTransformation.AddRolesFromClaim by extracting dedicated helpers for JSON parsing and delimited parsing to meet the cognitive complexity target defined in issue Refactor: Reduce Cognitive Complexity in AddRolesFromClaim #595.
  • Documented the new helpers with XML comments and preserved existing behavior, including the invalid JSON fallback path, in line with the project's contribution guidance.
  • Added focused unit coverage in RoleClaimsTransformationTests for JSON arrays, comma-separated strings, and malformed payloads so future changes remain safe.

Implementation Notes

  • JSON detection now relies on StringComparison.Ordinal to avoid culture-related surprises and mirrors other security components.
  • TryAddJsonArrayRoles returns false on any parsing failure to maintain the prior "best effort" semantics before falling back to string-based parsing.
  • The infrastructure test project now references FlowSynx directly so tests exercise the same class registered in production.

Testing

  • Attempted dotnet test tests/FlowSynx.Infrastructure.UnitTests/FlowSynx.Infrastructure.UnitTests.csproj --no-build (fails: dotnet CLI not available in execution environment)

Closes #595

@mohin-io mohin-io requested review from a team as code owners October 28, 2025 23:47
@sonarqubecloud
Copy link
Copy Markdown

Copy link
Copy Markdown
Member

@ziagham ziagham left a comment

Choose a reason for hiding this comment

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

The changes look good overall. Thanks.
However, the unit tests should have been placed in a separate project named FlowSynx.UnitTests. Although that project doesn’t currently exist and needs to be created, it’s not a major issue. I’ll create a separate task to add a new FlowSynx.UnitTests project and move all related tests there.

@ziagham ziagham merged commit 25c216a into flowsynx:master Oct 29, 2025
4 checks passed
@mohin-io
Copy link
Copy Markdown
Contributor Author

Hi @ziagham, thanks for appreciating my work!
I also think, it would be better to separate all the tests in FlowSynx.UnitTests.
I'm genuinely excited about FlowSynx and believe in its vision. Seeing the foundation you've built has really solidified my interest tbh. I'm fully committed to contributing my earnest efforts to help move this project forward.

@ziagham
Copy link
Copy Markdown
Member

ziagham commented Oct 29, 2025

@mohin-io
Thanks! I’m glad you share your excitement for FlowSynx. I’d be happy to have you contribute—if you want, I can add you as a GitHub collaborator so you can start pushing changes and help move the project forward!

@mohin-io
Copy link
Copy Markdown
Contributor Author

That's awesome, @ziagham ! Thank you for the trust and the opportunity.
I would be thrilled to become a collaborator. You can add me, and I'll start diving in. The future of FlowSynx is looking bright, and I'm excited to help build it. Let's keep the momentum going!

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.

Refactor: Reduce Cognitive Complexity in AddRolesFromClaim

2 participants