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

Support JSONSchema while producing message #919

Merged

Conversation

txs4444
Copy link
Contributor

@txs4444 txs4444 commented Nov 23, 2021

Currently it is not possible to produce message to a topic while JSON Schema is chosen as key or value schema.
For producing message to a topic RecordRepository is used. In case schema id was present AvroSchemaSerializer was used regardless of schema type.

RecordWithSchemaSerializerFactory was introduced to take care of fetching schema from schema registry and create proper serializer. AvroSerializer is no longer responsible for fetching schema itself - it is not longer a bean but it is created on demand for specific schema.

A test not related to the functionality (SchemaRegistryRepositoryTest#register) was change because it was influenced by adding schema to schema registry by new RecordRepositoryTest

@tchiotludo
Copy link
Owner

hi @txs4444 !
amazing hard work here !! thanks 👍
Will need some time to review and to test, since I never used schema registry with JSON, sorry for that !

@txs4444
Copy link
Contributor Author

txs4444 commented Nov 23, 2021

hi @tchiotludo,
You're welcome. Useful tool.

Yes, sure. Let me know if you need some help while testing this.

@tchiotludo
Copy link
Owner

First try seems to works quite well :

  • I've added the ability to create json schema here
  • I was able to produce with a json schema.
  • Json Schema is validated

Seems to be good work here.

Just the unit test failed, all the count is wrong since you have added a topic, can you fix it ?

@txs4444
Copy link
Contributor Author

txs4444 commented Dec 5, 2021

Sure, I'll fix them. Although it make take a while. When I build it locally only one test fails.

  org.akhq.controllers.SchemaControllerTest

    ✔ listApi() (2.2s)

  203 passing (2m 33s)
  4 pending
  1 failing


208 tests completed, 1 failed, 4 skipped
There were failing tests. See the report at: file:///home/ratatoskr/src/txs4444/akhq/build/reports/tests/test/index.html

Deprecated Gradle features were used in this build, making it incompatible with Gradle 8.0.

You can use '--warning-mode all' to show the individual deprecation warnings and determine if they come from your own scripts or plugins.

See https://docs.gradle.org/7.1/userguide/command_line_interface.html#sec:command_line_warnings

BUILD SUCCESSFUL in 5m 4s
22 actionable tasks: 20 executed, 2 up-to-date

I have to find what does make the difference here.

@tchiotludo
Copy link
Owner

look at here : https://github.com/tchiotludo/akhq/runs/4423640449?check_suite_focus=true
You will see all the test that failed

RecordRepositoryTest#produceAndConsumeRecordUsingJsonSchema created a topic while producing a message, this influenced the other tests because number of topic was different depending on the test order. Now topic for this test is created in KafkaTestCluster to have always the same number of topic for each test regardless of the test order.
@txs4444
Copy link
Contributor Author

txs4444 commented Dec 6, 2021

I fix some of the test - I hope all. You see it's hard because the test result depends on the test order - my RecordRepositoryTest#produceAndConsumeRecordUsingJsonSchema created new topic while producing a message as a result tests like TopicControllerTest failed.
Now I explicitly created this topic in KafkaTestCluster and updated the numbers there so all the test had the same topics regardless of the test order.

Could you please run the checks again? I'm feeling kind of blind because the tests passes locally but it is not necessary true for the checks on github.

@tchiotludo
Copy link
Owner

@txs4444, I know some kind of tricky this part ...
I need to definitely change this behaviour one day ...
But test passed now, thanks for this good one ;)

@tchiotludo tchiotludo merged commit a198839 into tchiotludo:dev Dec 6, 2021
@tchiotludo
Copy link
Owner

And it's merged, thanks a lot :)

@txs4444
Copy link
Contributor Author

txs4444 commented Dec 6, 2021

@tchiotludo I have some vague idea how to improve the tests a little bit. I’ll post it when I have some time to work on it and check if it makes sense.

Thanks for the testing :)

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.

2 participants