fix: resolve crash in TransformModuleFilesToModel if regular schema file passed instead

[fsedano]

Description

The change checks the metadata for the module, if it's null it returns the proper error.

References

Fixes #384

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

[fsedano]

@elbuo8 - Hi, would it be possible to have a review on this? It's impacting our production code and I rather not deploy a patched version, so would love having this fixed upstream. Thanks!

[rhamzeh]

@fsedano - do you require an error? What do you think about returning a transformation with the module name empty?

In general OpenFGA treats type defs with module name as an empty string as having no module attached

What if we validate that if there is metadata set, the schema version is the same as the one passed in? So if you are passing in 1.2 and in the file it states 1.1 we can error on that?

[fsedano]

@rhamzeh In general I'd prefer to have an error, since this is most probably a input error. If the user wants to accept both it could catch the error and use the regular API for that. If we just hide the error it will go unnoticed and it might have unexpected side effects.

The way this was caught in prod is because somebody by mistake commited the wrong file as a module. I rather error out to point the user about the mistake.

[rhamzeh]

@rhamzeh In general I'd prefer to have an error, since this is most probably a input error. If the user wants to accept both it could catch the error and use the regular API for that. If we just hide the error it will go unnoticed and it might have unexpected side effects.

The way this was caught in prod is because somebody by mistake commited the wrong file as a module. I rather error out to point the user about the mistake.

Thanks for elaborating, will approve this to unblock you, and in the future we'll add options to allow this use-case.

May I ask if you can also add the JS fix so that this passes the tests and we can merge?

The one I currently forsee is allowing folks to run and test and transform their module without worrying about the module file and then a separate platform process does the combining - but as that is not a current use-case, it shouldn't be a blocker.

Also can I ask how are you consuming this? Is it by calling it directly or through the CLI? (to see if we need to spin up a new CLI release that contains this fix to unblock you)

[fsedano]

Thanks @rhamzeh - Just fixed also the JS one, thanks for letting me know.
We are not using CLI, but the Go SDK, so no CLI hotfix is needed.

Thanks again!

[fsedano]

Thanks @rhamzeh - Is there anything else I have to do to have this merged or will it happen automagically? Thanks,

[rhamzeh]

Was waiting for all the tests to pass, just added it to the queue.

Thanks for the fix!