search

swagger-api / apidom

Semantic parser for API specifications
https://swagger-api.github.io/apidom/
72 stars 18 forks source link

Consolidate meta (metadata) information #867

Open char0n opened 3 years ago

char0n commented 3 years ago

These are the agreed rules on engagement about adding meta (metadata) to ApiDOM:

Action items:

frantuma commented 3 years ago

@char0n

In terms of identifying possible broken rules, I guess you're right in saying that there aren't any locations except the newly introduced spec-version where they could be moved to elements with concrete type. I will fix the spec-version thing shortly.

The main pattern where I see this could be an issue is actually for non concrete types: an example could be version or even better operation-message (as in this case for example we are setting metadata only in case this is not a reference, while even if it's a reference semantically this is still an operation message)

(or other similar calls to this.element.classes.. within refractor dir in namespace where we don't have a concrete element type)

In such cases to me the version or operation message fall into the meta information is static and generic to all usecases even if element is not of concrete type, but being non concrete I don't know if there's a way to set the metadata within the element instead of the visitor, as within the constructor of the parent children are not yet built, so not much to do I believe.

slightly related, specifically for cases like operation-message, here we are loosing semantic when we have a ref, IMO such meta should be added regardless of it being a ref or an object.

About version it make sense, will change it along with the fix for spec-version

char0n commented 3 years ago

Regarding non-concrete types (NCEs): I've introduced NCE classes both for AsyncAPI 2.2.0 and OpenAPI 3.1.0 specifications some time ago.

In such cases to me the version or operation message fall into the meta information is static and generic to all usecases even if element is not of concrete type, but being non concrete I don't know if there's a way to set the metadata within the element instead of the visitor, as within the constructor of the parent children are not yet built, so not much to do I believe.

Yep, not much to do here

The main pattern where I see this could be an issue is actually for non concrete types: an example could be version or even better operation-message (as in this case for example we are setting metadata only in case this is not a reference, while even if it's a reference semantically this is still an operation message)

This particular case will be converted to NCE in #916. I guess it depends on what you really want to achieve. My goal was following:

Operation can have either Reference, Array<Message> or Message elements. We have two CEs here and one NCE. I wanted to give NCE( Array<Message>) some additional semantics to distinguish it from other NCEs.

From what you described, your goal is to have unified metadata regardless if it's CE or NCE. This means that having defined NCE element class for this particular case is technically impossible and we need to add the metadata in refractor layer. This makes my goal subset of yours. If we really want to do this, it's going to be some amount of work refactoring the visitors and NCEs. But I currently don't have objections to it, as it makes sense.

Let's decide here, if the next action point should be the refactor. This would lead to having less NCE element classes defined in nces/ directories and more decoration in refractor layer.

char0n commented 2 years ago

@frantuma pinging about the decision here.

frantuma commented 2 years ago

@char0n I am not sure here, possibly it would be good to sync in a chat; I understand your points, let's say that possibly keeping things as they are could be also an option, following your original direction (static vs refactor, and defined NCEs). In your example above, we might want to add metadata in refractors for Array<Message> which is kind of "outside the pattern" but at least for the moment I would say that this is acceptable and also kind of more correct in terms of static vs "use case dependent"

char0n commented 2 years ago

Right, so if I understand correctly, we're keeping the status quo until we decide to change it in the future.