Skip to content

Conversation

@aaguiarz
Copy link
Member

@aaguiarz aaguiarz commented Nov 5, 2025

This fixes the issue reported in:
openfga/action-openfga-test#32

The bug occurred when the CLI v0.7.7 changed how the --tests parameter resolves paths. The model test command was passing both:

  • fileName: the full path from glob (e.g., "my_project/infrastructure/fga/tests.fga.yaml")
  • basePath: path.Dir(fileName) (e.g., "my_project/infrastructure/fga")

This caused ReadFromFile to join them, creating a duplicated path: "my_project/infrastructure/fga/my_project/infrastructure/fga/tests.fga.yaml"

The fix passes an empty basePath since the file path from glob is already complete. ReadFromFile now correctly uses the file's directory internally for resolving nested file references (model_file, tuple_file).

Changes:

  • Modified cmd/model/test.go to pass empty basePath to ReadFromFile
  • Added comprehensive unit tests in internal/storetest/read-from-input_test.go that verify the fix and prevent regression

Description

What problem is being solved?

How is it being solved?

What changes are made to solve it?

References

Review Checklist

  • I have clicked on "allow edits by maintainers".
  • I have added documentation for new/changed functionality in this PR or in a PR to openfga.dev [Provide a link to any relevant PRs in the references section above]
  • The correct base branch is being used, if not main
  • I have added tests to validate that the change in functionality is working as expected

Summary by CodeRabbit

  • Bug Fixes

    • Resolved path resolution issue that was causing incorrect path duplication in file handling.
  • Tests

    • Added comprehensive test coverage for file path resolution scenarios, including nested file references and error messaging.

This fixes the issue reported in:
openfga/action-openfga-test#32

The bug occurred when the CLI v0.7.7 changed how the --tests parameter
resolves paths. The model test command was passing both:
- fileName: the full path from glob (e.g., "my_project/infrastructure/fga/tests.fga.yaml")
- basePath: path.Dir(fileName) (e.g., "my_project/infrastructure/fga")

This caused ReadFromFile to join them, creating a duplicated path:
"my_project/infrastructure/fga/my_project/infrastructure/fga/tests.fga.yaml"

The fix passes an empty basePath since the file path from glob is already
complete. ReadFromFile now correctly uses the file's directory internally
for resolving nested file references (model_file, tuple_file).

Changes:
- Modified cmd/model/test.go to pass empty basePath to ReadFromFile
- Added comprehensive unit tests in internal/storetest/read-from-input_test.go
  that verify the fix and prevent regression
Copilot AI review requested due to automatic review settings November 5, 2025 13:58
@aaguiarz aaguiarz requested a review from a team as a code owner November 5, 2025 13:58
@linux-foundation-easycla
Copy link

linux-foundation-easycla bot commented Nov 5, 2025

CLA Not Signed

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Nov 5, 2025

Important

Review skipped

Auto incremental reviews are disabled on this repository.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Walkthrough

The PR fixes path duplication in test file resolution by changing how ReadFromFile combines the base path and file name. A new test file is added to validate the path resolution behavior under various scenarios, including the previous duplication bug and correct handling of empty, relative, and absolute paths.

Changes

Cohort / File(s) Summary
Test configuration update
cmd/model/test.go
Modifies the basePath argument passed to storetest.ReadFromFile from path.Dir(file) to an empty string, preventing path duplication when resolving nested test file references.
Path resolution test suite
internal/storetest/read-from-input_test.go
Adds comprehensive test coverage for ReadFromFile path resolution behavior, including tests for duplication prevention, empty basePath handling, absolute path resolution, nested references, and error message quality.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

  • The actual code change is minimal (single argument replacement in one file)
  • New test file is straightforward with clear test scenarios
  • No complex logic or structural changes involved
  • Verify test cases adequately cover the path resolution edge cases mentioned

Possibly related PRs

  • PR #516: Modifies ReadFromFile's path-resolution behavior for how basePath and fileName are combined and opened.
  • PR #515: Adjusts test file resolution in cmd/model/test.go and adds glob-based file resolution capabilities.

Suggested labels

codex

Suggested reviewers

  • rhamzeh
  • sergiught
  • Siddhant-K-code

Pre-merge checks and finishing touches

✅ Passed checks (2 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title accurately summarizes the main fix: preventing path duplication in the model test command by changing how basePath is passed to ReadFromFile.

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.

Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR fixes a path duplication bug in the model test command where passing path.Dir(file) as the basePath parameter to ReadFromFile caused the path to be joined incorrectly, creating duplicated paths like "path/to/file/path/to/file/test.yaml". The fix passes an empty basePath instead, allowing ReadFromFile to use the file's directory internally for resolving nested file references.

Key changes:

  • Modified cmd/model/test.go to pass an empty string as basePath to ReadFromFile
  • Added comprehensive test coverage in internal/storetest/read-from-input_test.go to validate the fix and prevent regression

Reviewed Changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.

File Description
cmd/model/test.go Changed ReadFromFile call to pass empty basePath instead of path.Dir(file), with detailed explanation comment
internal/storetest/read-from-input_test.go New test file covering path resolution scenarios including the bug case, correct usage with empty basePath, absolute paths, nested references, and error messages
Comments suppressed due to low confidence (1)

cmd/model/test.go:22

  • The path package import on line 22 is no longer used after removing the path.Dir(file) call. This import should be removed to keep the imports clean.
	"path"

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@Siddhant-K-code
Copy link
Member

@copilot can you fix the lint issues pls!

Copy link

Copilot AI commented Nov 5, 2025

@Siddhant-K-code I've opened a new pull request, #602, to work on those changes. Once the pull request is ready, I'll request review from you.

* Initial plan

* fix: remove unused 'path' import from cmd/model/test.go

Co-authored-by: Siddhant-K-code <55068936+Siddhant-K-code@users.noreply.github.com>

---------

Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com>
Co-authored-by: Siddhant-K-code <55068936+Siddhant-K-code@users.noreply.github.com>
@Siddhant-K-code
Copy link
Member

/easycla

@Siddhant-K-code
Copy link
Member

Ahh, we wanted to get the claude get whitelisted for EasyCLA

@ewanharris
Copy link
Member

Fixed in #603

@ewanharris ewanharris closed this Nov 5, 2025
@Siddhant-K-code Siddhant-K-code deleted the claude/fix-openfga-cli-issue-011CUpcTG9mCYkM5dxYdCnv2 branch November 5, 2025 16:38
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.

5 participants