Skip to content

Comments

Kafka Notifications: allow extraHeaders and prioritize override of Fiware-Service / Fiware-ServicePath #4726#4727

Merged
fgalan merged 15 commits intomasterfrom
feature/4726-allow-overwriting-Kafka-headers
Nov 28, 2025
Merged

Kafka Notifications: allow extraHeaders and prioritize override of Fiware-Service / Fiware-ServicePath #4726#4727
fgalan merged 15 commits intomasterfrom
feature/4726-allow-overwriting-Kafka-headers

Conversation

@orianar
Copy link
Collaborator

@orianar orianar commented Nov 9, 2025 •
edited by fgalan
Loading

Issue #4727 #4726

@orianar orianar requested a review from fgalan November 12, 2025 03:22
fgalan
fgalan reviewed Nov 12, 2025
View reviewed changes
..._kafka_notif_override_fiware_service_and_servicepath/s_custom_subscription_modification.test
Copy link
Member

@fgalan fgalan Nov 12, 2025

Choose a reason for hiding this comment

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

should be moved to (new) directory

test/functionalTest/cases/4726_kafka_notif_override_fiware_service_and_servicepath/

or some similar name

Copy link
Collaborator Author

@orianar orianar Nov 13, 2025

Choose a reason for hiding this comment

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

Fixed in #7c82da7

fgalan
fgalan reviewed Nov 12, 2025
View reviewed changes
src/lib/apiTypesV2/KafkaInfo.cpp
@@ -91,6 +91,11 @@ std::string KafkaInfo::toJson()
jh.addRaw("ngsi", this->ngsi.toJson(NGSI_V2_NORMALIZED, true));
Copy link
Member

@fgalan fgalan Nov 12, 2025

Choose a reason for hiding this comment

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

A new entry to CHANGES_NEXT_RELEASE file about the changes in this PR should be added.

Copy link
Collaborator Author

@orianar orianar Nov 13, 2025

Choose a reason for hiding this comment

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

Fixed in #7c82da7

fgalan
fgalan reviewed Nov 13, 2025
View reviewed changes
fgalan
fgalan reviewed Nov 13, 2025
View reviewed changes
..._kafka_notif_override_fiware_service_and_servicepath/s_custom_subscription_modification.test
"url": "kafka://localhost:9092,localhost:9094",
"topic": "sub1",
"payload": "{ %22A%22: %22${A}%22 }",
"headers": {"fiware-service": "cucu"}
Copy link
Member

@fgalan fgalan Nov 13, 2025

Choose a reason for hiding this comment

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

Test only include fiware-service and fiware-servicepath cases.

It would be great to include also a .test including a not-special header (i.e. a "random" header invented by the user).

Copy link
Collaborator Author

@orianar orianar Nov 14, 2025

Choose a reason for hiding this comment

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

Fixed in #f79c9f4

orianar and others added 3 commits November 13, 2025 13:56
@orianar @fgalan
Update CHANGES_NEXT_RELEASE
637ba92 
Co-authored-by: Fermín Galán Márquez <fgalan@users.noreply.github.com>
@orianar
add no_special_headers_subscription_modification.test
f79c9f4
@orianar
Merge branch 'feature/4726-allow-overwriting-Kafka-headers' of github…
5d603d0 
….com:telefonicaid/fiware-orion into feature/4726-allow-overwriting-Kafka-headers
fgalan
fgalan reviewed Nov 14, 2025
View reviewed changes
fgalan
fgalan reviewed Nov 14, 2025
View reviewed changes
fgalan
fgalan reviewed Nov 14, 2025
View reviewed changes
src/lib/ngsiNotify/Notifier.cpp Outdated
@@ -589,32 +625,13 @@ static SenderThreadParams* buildSenderParamsCustom
//
// 5. HTTP Headers (only in the case of HTTP notifications)
Copy link
Member

@fgalan fgalan Nov 14, 2025

Choose a reason for hiding this comment

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

Suggested change
// 5. HTTP Headers (only in the case of HTTP notifications)
// 5. Headers (only in the case of HTTP and Kafka notifications). In the case of HTTP, we use proper HTTP headers, in the case of Kakfa we use...

Need to end the sentence "we use..." :)

Copy link
Collaborator Author

@orianar orianar Nov 14, 2025

Choose a reason for hiding this comment

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

Fixed in #d507e52

fgalan
fgalan reviewed Nov 14, 2025
View reviewed changes
fgalan
fgalan reviewed Nov 14, 2025
View reviewed changes
@fgalan
Copy link
Member

fgalan commented Nov 14, 2025

Is the PR ready to merge (after review)? Should [WIP] be removed from title?

@orianar
Copy link
Collaborator Author

orianar commented Nov 14, 2025

Is the PR ready to merge (after review)? Should [WIP] be removed from title?

For now I think it’s fine to mix them; it meets the scope of this issue, but I have the feeling we’re missing tests for macro substitution (that could be done in a separate issue) and documentation for the Kafka headers. I’m not sure what the best place would be for that documentation.

@orianar
fix comment
d507e52
@orianar
Merge branch 'feature/4726-allow-overwriting-Kafka-headers' of github…
6b78788 
….com:telefonicaid/fiware-orion into feature/4726-allow-overwriting-Kafka-headers
@fgalan
Copy link
Member

fgalan commented Nov 17, 2025

...I have the feeling we’re missing tests for macro substitution (that could be done in a separate issue)

Maybe you can add these tests in a separate PR (letting opened this one until we see how they go)?

@fgalan
Copy link
Member

fgalan commented Nov 17, 2025 •
edited
Loading

...and documentation for the Kafka headers. I’m not sure what the best place would be for that documentation.

I agree. It is mostly clear for everyone what HTTP header and HTTP payload means, but not so sure in the Kafka world.

I mean, in the .test I see things like this:

Key: REGEX([0-9a-f\-]{24})
Headers:
  Fiware-Servicepath: /
  x-device-model: Model-1
  x-device-type: T
Payload:
{
    "A": "1"
}

But maybe is not so clear what "Key", "Headers" and "Payload" means from an encoding point of view.

The best place to include that documentation maybe is https://github.com/telefonicaid/fiware-orion/blob/master/doc/manuals/user/kafka_notifications.md. Maybe you can do a draft modification in this PR so I can provide feedback on it.

@orianar
Copy link
Collaborator Author

orianar commented Nov 28, 2025

...Tengo la sensación de que nos faltan pruebas de sustitución de macros (eso se podría hacer en un número aparte)

¿Tal vez puedas agregar estas pruebas en un PR separado (dejando éste abierto hasta que veamos cómo van)?

PR #4733

@orianar
Copy link
Collaborator Author

orianar commented Nov 28, 2025

...and documentation for the Kafka headers. I’m not sure what the best place would be for that documentation.

I agree. It is mostly clear for everyone what HTTP header and HTTP payload means, but not so sure in the Kafka world.

I mean, in the .test I see things like this:

Key: REGEX([0-9a-f\-]{24})
Headers:
  Fiware-Servicepath: /
  x-device-model: Model-1
  x-device-type: T
Payload:
{
    "A": "1"
}

But maybe is not so clear what "Key", "Headers" and "Payload" means from an encoding point of view.

The best place to include that documentation maybe is https://github.com/telefonicaid/fiware-orion/blob/master/doc/manuals/user/kafka_notifications.md. Maybe you can do a draft modification in this PR so I can provide feedback on it.

Fixed in #4879bfc

@orianar orianar changed the title [WIP] Kafka Notifications: allow extraHeaders and prioritize override of Fiware-Service / Fiware-ServicePath #4726 Kafka Notifications: allow extraHeaders and prioritize override of Fiware-Service / Fiware-ServicePath #4726 Nov 28, 2025
@orianar orianar requested a review from fgalan November 28, 2025 03:20
fgalan
fgalan reviewed Nov 28, 2025
View reviewed changes
fgalan
fgalan reviewed Nov 28, 2025
View reviewed changes
fgalan
fgalan reviewed Nov 28, 2025
View reviewed changes
fgalan
fgalan approved these changes Nov 28, 2025
View reviewed changes
Copy link
Member

@fgalan fgalan left a comment

Choose a reason for hiding this comment

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

LGTM

@fgalan fgalan merged commit 1cdc7a0 into master Nov 28, 2025
17 checks passed
@fgalan fgalan mentioned this pull request Nov 28, 2025
Closed
@fgalan
Copy link
Member

fgalan commented Nov 28, 2025

@fisuda this PR does some modificdations to English .md files. It would be great if you could synchronise the Japanese translation, please.

@fgalan fgalan deleted the feature/4726-allow-overwriting-Kafka-headers branch November 28, 2025 11:20
fisuda added a commit to fisuda/fiware-orion that referenced this pull request Nov 28, 2025
@fisuda
(JP) Kafka Notifications: allow extraHeaders and prioritize override …
7149fc8 
…of Fiware-Service / Fiware-ServicePath (telefonicaid#4727)
@fisuda
Copy link
Contributor

fisuda commented Nov 28, 2025

I sent the PR #4734.
Thanks.

fgalan added a commit that referenced this pull request Nov 28, 2025
@fgalan
Merge pull request #4734 from fisuda/patch
e12215a 
(JP) Kafka Notifications: allow extraHeaders and prioritize override of Fiware-Service / Fiware-ServicePath (#4727)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@fgalan fgalan fgalan approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@orianar @fgalan @fisuda