feat: added a new text embedded format #858

antonio-pedro99 · 2023-12-28T13:35:32Z

This fixes #739

Note: I closed #836 and started a fresh PR.

antonio-pedro99 · 2023-12-28T14:26:04Z

@ppatierno all tests are now passing.
I wonder why the Docs build is failing, as I did not change anything related to docs!

ppatierno · 2023-12-29T11:49:02Z

@antonio-pedro99 build is failing on doc with following error ...

ERROR: Uncommitted changes in documentation:
M	documentation/book/api/paths.adoc
Run the following to add up-to-date resources:
  make docu_api \
    && git add documentation/book/ \
    && git commit -s -m 'Update generated documentation'

antonio-pedro99 · 2023-12-29T15:19:37Z

@antonio-pedro99 build is failing on doc with following error ...

ERROR: Uncommitted changes in documentation:
M	documentation/book/api/paths.adoc
Run the following to add up-to-date resources:
  make docu_api \
    && git add documentation/book/ \
    && git commit -s -m 'Update generated documentation'


[INFO] BUILD FAILURE
[INFO] ------------------------------------------------------------------------
[INFO] Total time:  4.118 s
[INFO] Finished at: 2023-12-29T20:49:05+05:30
[INFO] ------------------------------------------------------------------------
[WARNING] 
[WARNING] Plugin validation issues were detected in 1 plugin(s)
[WARNING] 
[WARNING]  * io.github.swagger2markup:swagger2markup-maven-plugin:1.3.7
[WARNING] 
[WARNING] For more or less details, use 'maven.plugin.validation' property with one of the values (case insensitive): [BRIEF, DEFAULT, VERBOSE]
[WARNING] 
[ERROR] Failed to execute goal io.github.swagger2markup:swagger2markup-maven-plugin:1.3.7:convertSwagger2markup (generate-apidoc) on project kafka-bridge: Failed to execute goal 'convertSwagger2markup': Error creating extended parser class: Could not determine whether class 'org.pegdown.Parser$$parboiled' has already been loaded: Unable to make protected final java.lang.Class java.lang.ClassLoader.findLoadedClass(java.lang.String) accessible: module java.base does not "opens java.lang" to unnamed module @358ab600 -> [Help 1]
[ERROR] 
[ERROR] To see the full stack trace of the errors, re-run Maven with the -e switch.
[ERROR] Re-run Maven using the -X switch to enable full debug logging.
[ERROR] 
[ERROR] For more information about the errors and possible solutions, please read the following articles:
[ERROR] [Help 1] http://cwiki.apache.org/confluence/display/MAVEN/MojoFailureException
make: *** [Makefile:63: docu_api] Error 1

ppatierno · 2023-12-29T15:25:12Z

Just ran on your branch locally the command from Azure pipeline ...

mvn -e -V -B -P apidoc io.github.swagger2markup:swagger2markup-maven-plugin:convertSwagger2markup@generate-apidoc

and it works like a charm. It produces a documentation/book/api/paths.adoc file which you should push.

antonio-pedro99 · 2023-12-29T18:24:47Z

Just ran on your branch locally the command from Azure pipeline ...
mvn -e -V -B -P apidoc io.github.swagger2markup:swagger2markup-maven-plugin:convertSwagger2markup@generate-apidoc
and it works like a charm. It produces a documentation/book/api/paths.adoc file which you should push.

I need to double-check, it still does not work on my side

ppatierno · 2023-12-31T11:22:30Z

@antonio-pedro99 I have just pushed on your branch with the missing file. Please update your local branch checking out the changes.

antonio-pedro99 · 2023-12-31T19:20:24Z

@antonio-pedro99 I have just pushed on your branch with the missing file. Please update your local branch checking out the changes.

work done!
Waiting for more reviews

scholzj

@antonio-pedro99 Thanks for the PR. I wonder how the conversion / error handling works here. I guess it either does not work that well, or it should be documented how does it (not) work.

For example, when I sent a following produce request, where the format is text but the value is JSON, it seems to pass fine:

curl -v -X POST \
    192.168.1.220:8080/topics/kafka-test-apps \
    -H 'content-type: application/vnd.kafka.text.v2+json' \
    -d '{
      "records": [
          {
              "key": "tricky-payload",
              "value": {"Ahoj": "Svete"}
          }
      ]
  }'

But when I try to consume it, I get empty value:

curl -X GET http://192.168.1.220:8080/consumers/my-bridge-group/instances/my-http-client/records \
  -H 'accept: application/vnd.kafka.text.v2+json'
[
  {
    "topic": "kafka-test-apps",
    "key": "tricky-payload",
    "value": "",
    "partition": 0,
    "offset": 69
  }
]

This seems to be the issue with the producer here where the bridge decodes the JSON as empty string. When I produce it as JSON and consume as text, I get the more expected "value": "{\"Ahoj\":\"Svete\"}". Not sure this is a bug or expected. But if ti is expected, I thik it is worth documenting how to produce the messages.

The response seems to have also a wrong content type application/vnd.kafka.json.v2+json. That looks like something you have to fix in HttpSink BridgeEndpoint.pollHandler.

antonio-pedro99 · 2024-01-01T16:13:09Z

@antonio-pedro99 Thanks for the PR. I wonder how the conversion / error handling works here. I guess it either does not work that well, or it should be documented how does it (not) work.

For example, when I sent a following produce request, where the format is text but the value is JSON, it seems to pass fine:
curl -v -X POST \
    192.168.1.220:8080/topics/kafka-test-apps \
    -H 'content-type: application/vnd.kafka.text.v2+json' \
    -d '{
      "records": [
          {
              "key": "tricky-payload",
              "value": {"Ahoj": "Svete"}
          }
      ]
  }'
But when I try to consume it, I get empty value:
curl -X GET http://192.168.1.220:8080/consumers/my-bridge-group/instances/my-http-client/records \
  -H 'accept: application/vnd.kafka.text.v2+json'
[
  {
    "topic": "kafka-test-apps",
    "key": "tricky-payload",
    "value": "",
    "partition": 0,
    "offset": 69
  }
]
This seems to be the issue with the producer here where the bridge decodes the JSON as empty string. When I produce it as JSON and consume as text, I get the more expected "value": "{\"Ahoj\":\"Svete\"}". Not sure this is a bug or expected. But if ti is expected, I thik it is worth documenting how to produce the messages.

That is a good catch, tbh I am not sure how it is supposed to be handled, I implemented the converter following our discussions on #739. I think it's worth to think about how to handle this case. Allow me some time to think a bit more.

The response seems to have also a wrong content type application/vnd.kafka.json.v2+json. That looks like something you have to fix in HttpSink BridgeEndpoint.pollHandler.

This is fixed in the recent commit.

src/main/java/io/strimzi/kafka/bridge/http/HttpSinkBridgeEndpoint.java

ppatierno · 2024-01-02T09:27:47Z

I think what Jakub found is a bug and the way it should work is that you could produce using an embedded format but receiving with a different one by accepting that there could be some "differences" in what you get.
I mean, you could produce using JSON format and value being a JSON but then you should be able to consume using TEXT format and getting the value as a text not a json (in the end the json value is a .... text). Of course, taking into account the escape of quoting fields in the JSON value.

scholzj · 2024-01-02T09:54:03Z

I mean, you could produce using JSON format and value being a JSON but then you should be able to consume using TEXT format and getting the value as a text not a json (in the end the json value is a .... text). Of course, taking into account the escape of quoting fields in the JSON value.

I think that works fine in the consumer IIRC what I was trying. It is more what happens the producer with text format sends a JSON where we can maybe throw an error or something.

ppatierno · 2024-01-02T10:57:05Z

It is more what happens the producer with text format sends a JSON where we can maybe throw an error or something.

Regarding this, because the producer is sending with "text" format and it's a JSON, why and how the bridge should know to validate it's a valid JSON. I mean, it's a plain text. Of course, it can contain a JSON but does it really matter for the bridge? It's just bringing text.

scholzj · 2024-01-02T11:28:11Z

I guess you can check if the text is empty/null if there are any other nodes? Anyway, as I said, maybe also just clarifying it in the docs would be fine.

…

On Tue, Jan 2, 2024, 11:57 Paolo Patierno ***@***.***> wrote: It is more what happens the producer with text format sends a JSON where we can maybe throw an error or something. Regarding this, because the producer is sending with "text" format and it's a JSON, why and how the bridge should know to validate it's a valid JSON. I mean, it's a plain text. Of course, it can contain a JSON but does it really matter for the bridge? It's just bringing text. — Reply to this email directly, view it on GitHub <#858 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ABLFOR7VP5ZWKDINW7U3ZLTYMPRYXAVCNFSM6AAAAABBFQRUSSVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTQNZTHA3TQNZUHE> . You are receiving this because your review was requested.Message ID: ***@***.***>

antonio-pedro99 · 2024-01-04T11:57:10Z

Hey @ppatierno @scholzj, just got busy with some work here. I will get back to you guys over the weekend.

ppatierno · 2024-01-16T08:30:19Z

@antonio-pedro99 are you willing to address this soon?

antonio-pedro99 · 2024-01-16T08:31:14Z

@antonio-pedro99 are you willing to address this soon?

Yes , I will!

antonio-pedro99 · 2024-01-16T08:31:49Z

Are you planning any release soon?

ppatierno · 2024-01-16T08:33:35Z

Are you planning any release soon?

No specific plan for now, I was just wondering if you were still interested in it because of 2 weeks after your last comment :-)

antonio-pedro99 · 2024-01-16T08:37:51Z

Are you planning any release soon?

No specific plan for now, I was just wondering if you were still interested in it because of 2 weeks after your last comment :-)

Yeah, I try to complete this on this weekend.

ppatierno · 2024-01-31T08:11:48Z

@antonio-pedro99 any news?

antonio-pedro99 · 2024-01-31T23:46:06Z

Not yet :( 😞 It's been hard handling the time. Once there is an update, I'll let you know.

…

On Wed, 31 Jan, 2024, 1:42 pm Paolo Patierno, ***@***.***> wrote: @antonio-pedro99 <https://github.com/antonio-pedro99> any news? — Reply to this email directly, view it on GitHub <#858 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AKFSX3F3KZSFJPAV2VD4JYLYRH4FBAVCNFSM6AAAAABBFQRUSSVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTSMJYGU4TGNBWGE> . You are receiving this because you were mentioned.Message ID: ***@***.***>

antonio-pedro99 · 2024-02-03T19:47:13Z

I mean, you could produce using JSON format and value being a JSON but then you should be able to consume using TEXT format and getting the value as a text not a json (in the end the json value is a .... text). Of course, taking into account the escape of quoting fields in the JSON value.

I think that works fine in the consumer IIRC what I was trying. It is more what happens the producer with text format sends a JSON where we can maybe throw an error or something.

what is consumer IIRC?

antonio-pedro99 · 2024-02-03T19:52:57Z

It is more what happens the producer with text format sends a JSON where we can maybe throw an error or something.

Regarding this, because the producer is sending with "text" format and it's a JSON, why and how the bridge should know to validate it's a valid JSON. I mean, it's a plain text. Of course, it can contain a JSON but does it really matter for the bridge? It's just bringing text.

So you suggest validating the JSON and make the possible conversion?

If the format is text, it makes more sense to expect text only.

scholzj · 2024-02-28T07:50:11Z

Looks like the build failed on checkstyle:

[ERROR] /home/vsts/work/1/s/src/main/java/io/strimzi/kafka/bridge/http/converter/HttpTextMessageConverter.java:27:5: NPath Complexity is 217 (max allowed is 200). [NPathComplexity]

Maybe you can have a look if the code can be fixed - if not, you should be able to use @SuppressWarnings("checkstyle:NPathComplexity") annotation to override the warning. The code does not seem to be somehow super bad in that file.

Signed-off-by: Antonio Pedro <[email protected]>

Signed-off-by: Paolo Patierno <[email protected]> Signed-off-by: Antonio Pedro <[email protected]>

Signed-off-by: Antonio Pedro <[email protected]>

This reverts commit b009700. Signed-off-by: Antonio Pedro <[email protected]>

Signed-off-by: Antonio Pedro <[email protected]>

antonio-pedro99 · 2024-03-04T07:29:27Z

@antonio-pedro99 I think you had problem with rebase. There are changes which are already in main, see all the "endpoint" classes.

Sorry, might be fixed now!

ppatierno

LGTM. Thanks for the PR!
@scholzj do you have anything to add?

scholzj

LGTM. But I guess we should have CHANGELOG record for this as well?

ppatierno · 2024-03-04T09:26:22Z

LGTM. But I guess we should have CHANGELOG record for this as well?

Right. @antonio-pedro99 can you add a line in the CHANGELOG about this new feature?

Signed-off-by: Antonio Pedro <[email protected]>

CHANGELOG.md

Signed-off-by: António Pedro <[email protected]>

ppatierno · 2024-03-04T17:01:08Z

@scholzj any further comments or can I merge this?

antonio-pedro99 requested a review from ppatierno December 28, 2023 14:22

antonio-pedro99 force-pushed the add-text-format branch from 055fd15 to 6f0d071 Compare December 28, 2023 14:56

antonio-pedro99 requested a review from scholzj December 29, 2023 10:39

ppatierno added this to the 0.28.0 milestone Dec 31, 2023

scholzj reviewed Dec 31, 2023

View reviewed changes

ppatierno requested changes Jan 2, 2024

View reviewed changes

src/main/java/io/strimzi/kafka/bridge/http/HttpSinkBridgeEndpoint.java Outdated Show resolved Hide resolved

antonio-pedro99 force-pushed the add-text-format branch from e590318 to 7b3fa3f Compare February 27, 2024 23:44

antonio-pedro99 force-pushed the add-text-format branch from b009700 to 115c84d Compare March 4, 2024 07:19

antonio-pedro99 and others added 15 commits March 4, 2024 12:57

feat: added a new text embedded format

305d1e9

Signed-off-by: Antonio Pedro <[email protected]>

test: added tests

729f164

Signed-off-by: Antonio Pedro <[email protected]>

doc: updated docs for the new format

d443530

Signed-off-by: Antonio Pedro <[email protected]>

Update generated documentation

908721b

Signed-off-by: Paolo Patierno <[email protected]> Signed-off-by: Antonio Pedro <[email protected]>

add get content type to match on Poll request

928c8be

Signed-off-by: Antonio Pedro <[email protected]>

handling incorrect value

1c8c0ef

Signed-off-by: Antonio Pedro <[email protected]>

added test

f6434d7

Signed-off-by: Antonio Pedro <[email protected]>

remove unused method

0647131

Signed-off-by: Antonio Pedro <[email protected]>

replace depracted put in ObjectNode object

f81952c

Signed-off-by: Antonio Pedro <[email protected]>

replace depracted put in ObjectNode object

0cc8987

Signed-off-by: Antonio Pedro <[email protected]>

added test

0d32846

Signed-off-by: Antonio Pedro <[email protected]>

remove unused method

fa6e59e

Signed-off-by: Antonio Pedro <[email protected]>

rebase with upstream/main

4ef22a1

Signed-off-by: Antonio Pedro <[email protected]>

Revert "rebase with upstream/main"

abcedf1

This reverts commit b009700. Signed-off-by: Antonio Pedro <[email protected]>

change serializer and content type

48d136e

Signed-off-by: Antonio Pedro <[email protected]>

antonio-pedro99 force-pushed the add-text-format branch from 2dd3965 to 48d136e Compare March 4, 2024 07:28

ppatierno approved these changes Mar 4, 2024

View reviewed changes

scholzj approved these changes Mar 4, 2024

View reviewed changes

added missing docs and CHANGELOG

f8144fa

Signed-off-by: Antonio Pedro <[email protected]>

ppatierno reviewed Mar 4, 2024

View reviewed changes

CHANGELOG.md Outdated Show resolved Hide resolved

antonio-pedro99 commented Mar 4, 2024

View reviewed changes

CHANGELOG.md Outdated Show resolved Hide resolved

added missing word xD

71bb868

Signed-off-by: António Pedro <[email protected]>

scholzj merged commit c3e96c7 into strimzi:main Mar 4, 2024
13 checks passed

antonio-pedro99 deleted the add-text-format branch March 4, 2024 17:24

antonio-pedro99 self-assigned this Mar 12, 2024

antonio-pedro99 mentioned this pull request Jul 20, 2024

feat: added support for message timestamp #915

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: added a new text embedded format #858

feat: added a new text embedded format #858

antonio-pedro99 commented Dec 28, 2023

antonio-pedro99 commented Dec 28, 2023 •

edited

Loading

ppatierno commented Dec 29, 2023

antonio-pedro99 commented Dec 29, 2023 •

edited

Loading

ppatierno commented Dec 29, 2023

antonio-pedro99 commented Dec 29, 2023

ppatierno commented Dec 31, 2023

antonio-pedro99 commented Dec 31, 2023

scholzj left a comment

antonio-pedro99 commented Jan 1, 2024

ppatierno commented Jan 2, 2024

scholzj commented Jan 2, 2024

ppatierno commented Jan 2, 2024

scholzj commented Jan 2, 2024 via email

antonio-pedro99 commented Jan 4, 2024

ppatierno commented Jan 16, 2024

antonio-pedro99 commented Jan 16, 2024

antonio-pedro99 commented Jan 16, 2024

ppatierno commented Jan 16, 2024

antonio-pedro99 commented Jan 16, 2024

ppatierno commented Jan 31, 2024

antonio-pedro99 commented Jan 31, 2024 via email

antonio-pedro99 commented Feb 3, 2024

antonio-pedro99 commented Feb 3, 2024

scholzj commented Feb 28, 2024 •

edited

Loading

antonio-pedro99 commented Mar 4, 2024

ppatierno left a comment

scholzj left a comment

ppatierno commented Mar 4, 2024

ppatierno commented Mar 4, 2024

feat: added a new text embedded format #858

feat: added a new text embedded format #858

Conversation

antonio-pedro99 commented Dec 28, 2023

antonio-pedro99 commented Dec 28, 2023 • edited Loading

ppatierno commented Dec 29, 2023

antonio-pedro99 commented Dec 29, 2023 • edited Loading

ppatierno commented Dec 29, 2023

antonio-pedro99 commented Dec 29, 2023

ppatierno commented Dec 31, 2023

antonio-pedro99 commented Dec 31, 2023

scholzj left a comment

Choose a reason for hiding this comment

antonio-pedro99 commented Jan 1, 2024

ppatierno commented Jan 2, 2024

scholzj commented Jan 2, 2024

ppatierno commented Jan 2, 2024

scholzj commented Jan 2, 2024 via email

antonio-pedro99 commented Jan 4, 2024

ppatierno commented Jan 16, 2024

antonio-pedro99 commented Jan 16, 2024

antonio-pedro99 commented Jan 16, 2024

ppatierno commented Jan 16, 2024

antonio-pedro99 commented Jan 16, 2024

ppatierno commented Jan 31, 2024

antonio-pedro99 commented Jan 31, 2024 via email

antonio-pedro99 commented Feb 3, 2024

antonio-pedro99 commented Feb 3, 2024

scholzj commented Feb 28, 2024 • edited Loading

antonio-pedro99 commented Mar 4, 2024

ppatierno left a comment

Choose a reason for hiding this comment

scholzj left a comment

Choose a reason for hiding this comment

ppatierno commented Mar 4, 2024

ppatierno commented Mar 4, 2024

antonio-pedro99 commented Dec 28, 2023 •

edited

Loading

antonio-pedro99 commented Dec 29, 2023 •

edited

Loading

scholzj commented Feb 28, 2024 •

edited

Loading