Skip to content
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

Add option to hide successful tuple imports and display import summary #437

Merged

Conversation

Siddhant-K-code
Copy link
Contributor

@Siddhant-K-code Siddhant-K-code commented Jan 8, 2025

Description

This PR adds a new flag --hide-imported-tuples to the tuple write command that improves the output handling for large tuple imports.

tl;dr; Changes:

  • Add --hide-imported-tuples flag to suppress successful import output
  • Return total_count, failed_count, successful_count processed
  • Always show failed tuples even when hiding successful ones (for debugging purpose)
  • Add a test for the new functionality

Example usage:

fga tuple write --store-id=01H0H015178Y2V4CX10C2KGHF4 --file tuples.json --hide-imported-tuples

Example outputs:

With successful imports (without --hide-imported-tuples):

{
  "successful": [
    {
      "user": "user:anne",
      "relation": "viewer",
      "object": "document:roadmap"
    }
  ],
  "failed_count":0,
  "successful_count":1,
  "total_count":1
}

With failed imports:

{
  "failed": [
    {
      "tuple_key": {
        "user": "user:carl",
        "relation": "writer",
        "object": "document:roadmap"
      },
      "reason": "Write validation error ..."
    }
  ],
  "failed_count":1,
  "successful_count":0,
  "total_count":1
}

References

fixes #376

Review Checklist

@Siddhant-K-code Siddhant-K-code requested a review from a team as a code owner January 8, 2025 17:37
@aaguiarz
Copy link
Member

aaguiarz commented Jan 8, 2025

Thanks a lot @Siddhant-K-code!

Can you change it to output something like:

{
"total_count": 24,
"failed": [
{
"tuple_key": {
"object":"document:roadmap",
"relation":"writer",
"user":"carl"
},
"reason":"Write validation error ..."
}
]
}

So, we always detail the errors, as they are valuable, and output to the total count.

@Siddhant-K-code
Copy link
Contributor Author

@aaguiarz do you think, we should remove the separate txt summary from below?

@aaguiarz
Copy link
Member

aaguiarz commented Jan 8, 2025

Yes, let's remove it, so we keep it a json output that can be easily parsed if needed

@Siddhant-K-code

This comment was marked as outdated.

@Siddhant-K-code
Copy link
Contributor Author

lint fixes 🫠

@Siddhant-K-code
Copy link
Contributor Author

Siddhant-K-code commented Jan 8, 2025

💭 total_count could be ambiguous, I think we can also return successful_count & failed_count with it (everytime). Wdyt @aaguiarz ?

@aaguiarz
Copy link
Member

aaguiarz commented Jan 8, 2025

@Siddhant-K-code yeah, I like that more :)

@Siddhant-K-code
Copy link
Contributor Author

@aaguiarz, We are good to go now 🚀

do you think, we should also add this to the documentation?

@aaguiarz
Copy link
Member

aaguiarz commented Jan 9, 2025

@Siddhant-K-code yes, it should be added to the documentation, thanks in advance for that!

@Siddhant-K-code
Copy link
Contributor Author

@aaguiarz, I've raised the docs PR, here: openfga/openfga.dev#926

@Siddhant-K-code
Copy link
Contributor Author

I think, we are good to release it now, after release, we can merge the docs PR.
Let me know if there are any blockers on this one.

cmd/tuple/write.go Outdated Show resolved Hide resolved
cmd/tuple/write_test.go Outdated Show resolved Hide resolved
ewanharris
ewanharris previously approved these changes Jan 22, 2025
Copy link
Member

@ewanharris ewanharris left a comment

Choose a reason for hiding this comment

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

This looks good, thanks @Siddhant-K-code!

Would you mind squashing into a single commit?

Copy link

Minder Vulnerability Report ✅

Minder analyzed this PR and found it does not add any new vulnerable dependencies.

Vulnerability scan of bd62defd:

  • 🐞 vulnerable packages: 0
  • 🛠 fixes available for: 0

@Siddhant-K-code Siddhant-K-code force-pushed the feature/hide-imported-tuples-output branch from bd62def to ad65fad Compare January 22, 2025 10:01
@Siddhant-K-code Siddhant-K-code force-pushed the feature/hide-imported-tuples-output branch from 3d32f9d to 0f2bab5 Compare January 22, 2025 10:02
@Siddhant-K-code
Copy link
Contributor Author

@ewanharris, it is good to go now 🚀

@ewanharris ewanharris added this pull request to the merge queue Jan 22, 2025
Merged via the queue into openfga:main with commit ce8af82 Jan 22, 2025
15 checks passed
@Siddhant-K-code Siddhant-K-code deleted the feature/hide-imported-tuples-output branch January 22, 2025 10:55
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.

fga write --file should not always display the imported tuples
3 participants