openfga / language

Grammar for the OpenFGA modeling language
https://openfga.dev
Apache License 2.0
15 stars 7 forks source link

feat: add java package #139

Closed le-yams closed 4 months ago

le-yams commented 5 months ago

Initialize Java package

Description

References

closes #138

Review Checklist

le-yams commented 5 months ago

This first commit just add the java project that "receives" the generated code (there is no other code/test for now). I tried to keep the same tooling than the java-sdk project.

le-yams commented 5 months ago

@rhamzeh I added the tests stack to run all the "dsltojson", "jsontodsl" and "dslvalidator" tests based on what was written in the go and js packages. All tests are failing for now as nothing is implemented yet ^^

le-yams commented 5 months ago

I also imported the code I worked on a couple months ago, 98 over 263 tests pass (especially conditions and mixed operators are not implemented at all).

rhamzeh commented 5 months ago

Overall, it looks really good! Thanks @le-yams!

I'm still going over the code, but I see no problems with it so far. It's unfortunate Java is so verbose, but we have to live with that.

Will ask the rest of the folks to give it a look too!

P.S. When you have time, can you add a workflow file for Java (see go workflow file), just so that we can follow the progress here!

le-yams commented 5 months ago

can you add a workflow file for Java

I've just pushed it (I'm not very used to gradle and github workflows so it probably needs some tweaking ^^).

rhamzeh commented 5 months ago

@le-yams - just jumping in to say I love your progress on this and am very excited to see it when ready!

le-yams commented 5 months ago

Thanks @rhamzeh :)

I'm close to getting all tests green. I currently am strugling with two final tests that stay red. The two tests seems to be about the same thing (first for the dsl to json transformation, second for the dsl validation).

Here is a test output:

Expecting message to be:
  "3 errors occurred:
    * syntax error at line=1, column=0: extraneous input 'type' expecting {WHITESPACE, '#', 'model', NEWLINE}
    * syntax error at line=1, column=5: extraneous input 'user' expecting {'#', 'model', NEWLINE}
    * syntax error at line=2, column=0: mismatched input '<EOF>' expecting {'#', 'model'}

"
but was:
  "2 errors occurred:
    * syntax error at line=1, column=0: extraneous input 'type' expecting {WHITESPACE, '#', 'model', NEWLINE}
    * syntax error at line=1, column=5: mismatched input 'user' expecting {'#', 'model', NEWLINE}

"

Could it be related to the following comment present in the go dsltojson_test.go file?

_, err := transformer.TransformDSLToJSON(testCase.DSL)

errorsCount := len(testCase.ExpectedErrors)
if errorsCount == 0 {
  require.NoError(t, err)
} else {
  require.Error(t, err)

  // unfortunately antlr is throwing different error messages in Go and JS - considering that at the moment
  //  we care that it errors for syntax errors more than we care about the error messages matching,
  //  esp. in Go as we are not building a language server on top of the returned errors yet
  //  actual matching error strings is safe to ignore for now
  // require.EqualErrorf(t, err, testCase.GetErrorString(), "")
}

Because if I comment the size and messages related assertions (only keeping the error/no error check) it goes green.

rhamzeh commented 4 months ago

Also - FYI we're working on modular models in this branch https://github.com/openfga/language/tree/feat/148-support-modular-models, which implements the ideas documented in the Modular Models RFC and in the language epic related to that.

le-yams commented 4 months ago

Hey @rhamzeh :)

Also, I checked and it seemed the antlr files have not been regenerated, based on the latest antlr grammar in this branch (it might also be worth merging main and regenerating based of that)

Yes, I haven't worked on this branch in a while, it definitively needs a rebase and the grammar to be regenerated.

Finally, I may be doing something wrong but the tests keep failing for me locally

Not sure if it's the tests your talking about, there indeed was remaining failing tests and I was asking in my previous comment about it ^^.

I'll apply the differents suggestions you made and start to work on rebasing the branch :)

rhamzeh commented 4 months ago

Unfortunately CI is still failing, looking into it.

Not sure if it's the tests your talking about, there indeed was remaining failing tests and I was asking in my previous comment about it

I don't think that's it, in my test run there were 203 tests failing 😕

le-yams commented 4 months ago

DslToJsonShould > verifyDslSyntax(String, String, Collection) > no model header FAILED java.lang.AssertionError at DslToJsonShould.java:49

DslValidatorShould > verifyDslSyntax(String, String, List, boolean) > no model header FAILED org.opentest4j.AssertionFailedError at DslValidatorShould.java:48

267 tests completed, 2 failed

@rhamzeh it seems that the CI has the same result as me and only 2 tests are failing 😃.

So should I make the assertion less precise to make them pass? (cf this comment)

rhamzeh commented 4 months ago

Yep, let's! Then we can get it merged 🎉

le-yams commented 4 months ago

@rhamzeh I did the same as in the go tests, I commented the assertions and added the same explaining comment

rhamzeh commented 4 months ago

Super excited to have this finally in! Thank you @le-yams! ❤️