feat: enable additional properties for topicConfigurationObject

[gokerakc]

Description

• Updated additionalProperties as true for the topicConfigurationObject due to the existence of several vendor-specific configuration properties (e.g. confluent.schema.validation, confluent.value.subject.name.strategy, confluent.key.subject.name.strategy).
• This change was proposed in one of the old issues and it was approved but I believe the additionalProperties bit got missed by mistake.
• Please let me know if there is any obstacle to enabling that.

[dalelane]

makes sense to me - nice catch, thanks

[gokerakc]

Quick question, do we need to increase the version of the binding that is presented here?

[lbroudoux]

Hey there!

I understand the general purpose of enabling additional properties, but - for this specific concern of schema and registry-related things, we already have schemaRegistryVendor, schemaLookupStrategy and others schema* properties.
Shouldn't we try to encourage using those (maybe adding some missing stuff) in a more vendor-neutral and generic way?

BTW: we'll also have to report this additionalProperties activation to the v3 json-schemas as well.

[KhudaDad414]

LGTM!

[derberg]

@dalelane @lbroudoux folks, today evening we merge next-major-spec to master so if you want to merge this one before, this is the right time.

[derberg]

and if it comes to versioning, yes, imho it is new version because you also need to adjust kafka binding readme -> https://github.com/asyncapi/bindings/tree/master/kafka#topicconfiguration-object as now it says that only "listed" properties are allowed, and now you change that it can be extended with more properties.

anyway, not much time left 😉

[gokerakc]

Thanks for the comments everyone! I've updated the kafka bindings readme file.

@lbroudoux I see your point and also I am in favour of keeping the properties vendor-neutral. In our use case, we need to specify some of the Confluent topic configurations listed in here. For schema-related properties, we could follow what you have suggested but there are too many other configurations we need to consider. So I think it would be better to allow additional properties to give some flexibility.

@derberg do you think we can get this merged now?

[lbroudoux]

Hey @gokerakc!

I totally agree that we should not add all the available confluent-specific props, and your proposal makes sense.

Still, shouldn't we add something like schemaLookupStrategyForKey to cover confluent.key.subject.name.strategy that is missing here? That way, I think that defined properties may cover a vast majority of concerns that are common to vendors.

@dalelane What do you think?

[gokerakc]

Thanks @lbroudoux!

IMO, the new property schemaLookupStrategyForKey sounds good but if we need to be quick because of the next-major-spec branch merging work I would say let's add the new properties in another PR so we can have more time for thinking about naming and also possible other properties.

[derberg]

Hey folks, looks like there is not 💯 consensus, so we had a chat and do not want to rush here and merge this one as it would complicate life a bit -> https://asyncapi.slack.com/archives/C0230UAM6R3/p1701202455849719 (to join slack go to https://asyncapi.com/slack-invite)

Please continue discussion after we release v3 and improve what you need. From perspective of AsyncAPI v2 bindings were "experimental" and we did not validate them - AsyncAPI 3.0.0 is the first release where specification JSON Schema file includes bindings JSON Schema. So tooling do not block you from using additional properties for kafka binding in AsyncAPI v2 documents

[gokerakc]

I am closing this PR since it requires 💯 consensus and also because of the v3 updates some changes need to be moved to the schema repo.

I've created a new issue to discuss about this change. Please check this issue and share your thoughts so we can move forward. @lbroudoux @derberg @KhudaDad414 @dalelane

Thanks for your help!