AsyncAPI 3.x: parse servers, bindings, channel parameters and security - #1708
AsyncAPI 3.x: parse servers, bindings, channel parameters and security#1708LautaroPetaccio wants to merge 4 commits into
Conversation
514f933 to
cd10ae3
Compare
cd10ae3 to
8c12268
Compare
8c12268 to
5394ef7
Compare
|
|
||
| public Builder address(String address) { this.address = address; return this; } | ||
|
|
||
| public Builder servers(List<String> servers) { this.servers = servers; return this; } |
There was a problem hiding this comment.
can servers be null ? Add Objects.requireNonNull() for all cases where the expected type is non-null
There was a problem hiding this comment.
Not from a document. The spec makes it optional, and the parser folds an absent servers into an empty list before the model sees it. Added requireNonNull anyway on every required setter and builder argument, since these are public API on a module meant to become a library.
| public Builder servers(List<String> servers) { this.servers = servers; return this; } | ||
|
|
||
| public Builder messageKeys(Map<String, String> messageKeys) { | ||
| this.messageKeys = messageKeys; |
There was a problem hiding this comment.
Same as above, done.
| this.amqpQueue = builder.amqpQueue; | ||
| this.amqpExchange = builder.amqpExchange; | ||
| this.wsMethod = builder.wsMethod; | ||
| this.raw = Collections.unmodifiableMap(builder.raw); |
There was a problem hiding this comment.
can builder.raw be null?
There was a problem hiding this comment.
Not today (the parser always passes a fresh map), but the setter now rejects it explicitly, so the constructor can't be reached with one.
| @@ -49,6 +64,8 @@ private AsyncApiOperation(Builder builder) { | |||
| this.channelName = builder.channelName; | |||
| this.messageIds = Collections.unmodifiableList(builder.messageIds); | |||
There was a problem hiding this comment.
Same, never from the parser, now guarded at the setter.
| } | ||
|
|
||
| Map<String, AsyncApiServerVariable> variables = new LinkedHashMap<>(); | ||
| for (Map.Entry<String, JsonNode> entry : objectFieldsOf(node.get("variables")).entrySet()) { |
There was a problem hiding this comment.
add local variable to node.get("variables").
There was a problem hiding this comment.
Done, and for the other four section loops that read the same way (channels, operations, servers, channel messages), so they all read alike.
| JsonNode variable = entry.getValue(); | ||
| variables.put(entry.getKey(), new AsyncApiServerVariable( | ||
| entry.getKey(), | ||
| scalarOf(variable.get("default")), |
There was a problem hiding this comment.
add string constants for all strings ("default", "enum", etc)
There was a problem hiding this comment.
Done for all 53 keywords the parser reads, not just these.
| @@ -147,18 +167,29 @@ public static AsyncApiDocument parse( | |||
| Map<String, AsyncApiOperation> operations = new LinkedHashMap<>(); | |||
| for (Map.Entry<String, JsonNode> entry : objectFieldsOf(root.get("operations")).entrySet()) { | |||
There was a problem hiding this comment.
add variable to root.get("operations")
a952f2c to
31585de
Compare
The last of the document a client needs: how to reach the service, as opposed to what to say to it. - AsyncApiServer, with host, protocol, protocolVersion, pathname and variables. The protocol is also how AsyncAPI distinguishes the two incompatible AMQPs -- "amqp" is 0-9-1, and 1.0 is the separate "amqp1". Many published documents declare no server at all, and that is not an error: they describe the contract and leave the broker to deployment. - AsyncApiChannelBindings. Only the few fields that change what a client has to do are lifted out; everything else stays in a raw map, so nothing is lost and a later transport can read it without the model growing first. The Kafka binding matters most: it carries its own topic, and when present that is the topic a client uses rather than the channel address. Hence effectiveAddress(protocol), which prefers it for Kafka only and leaves the declared address untouched so the document still reads as written. - Channel parameters, kept verbatim. Resolving a templated address to a concrete topic is a run-time concern, not a parsing one. - AsyncApiSecurityScheme. Its type is a free-form string rather than an enum on purpose: AsyncAPI's catalogue is broad and broker-specific, and parsing must not fail on a scheme that is valid but that no transport here can use yet. Schemes may also be written inline where they are used rather than in components, and those are registered under a name derived from where they appear. Bindings and parameters are dereferenced. Reading a binding without following its $ref would leave a channel on its declared address while the real topic sat in components, which is a wrong answer rather than a missing one.
31585de to
4a29cf6
Compare
Review feedback: reading root.get(...) inline in a for-header is dense. Each of the five section loops now takes the node it iterates into a local first. Two were pointed out; all five are done, so they read alike.
Review feedback: the field names the parser reads were literals at each of the 69 places they are read. They are now constants, gathered in one nested Keyword holder so the field name is written once and what the parser understands can be seen in one place. "default", "enum" and the rest were pointed out; all 53 are done, since leaving any as literals would keep the smell being fixed. The holder is nested rather than flat so that the names read as what they are at the use site -- node.get(Keyword.PAYLOAD) -- and so that the keyword "defaultContentType" cannot be confused with AsyncApiDocument's fallback value of the same name, which sits two lines away from where it is read. The $ref pointer prefixes are composed from the keywords and from the resolver's separator constants, so the shape of a pointer into channels, servers or components is written down once rather than spelled out per prefix. No behaviour changes.
Review feedback asked whether the collections handed to the builders can be null. From a document they cannot: the specification makes every one of them optional, and the parser folds an absent field into an empty collection before the model sees it -- empty *is* the specification's "absent", as with a channel that names no servers being available on all of them. The parser is also the only caller, and every value it passes is a fresh collection. So the guard is a contract on a public API, not a fix for a live bug. The builders are the surface of a module meant to become a library, and a null handed to one of them used to fail later, inside Collections.unmodifiableList in the constructor, with a stack trace pointing at the model rather than at the caller. Objects.requireNonNull in the setter fails at the call site and names the field. Applied to everything the model requires: every collection-typed setter and the bindings object, and the arguments a model object is meaningless without -- a channel's name, an operation's name, action and channel, a message's id, name and content type, a server's name, host and protocol, a document's text, location and version, a security scheme's name and type, a correlation id's expression, source and pointer. Fields the documentation already describes as nullable, such as a channel's address or a message's payload, are left as they are. AsyncApiChannelTest pins the contract on the class the review pointed at.
Fifth in the AsyncAPI stack, on top of #3.
The previous PRs read what a service can be sent and where those messages travel. This adds the rest of what a client needs: how to reach the service.
What it adds
AsyncApiServer— host, protocol,protocolVersion,pathnameand variables. The protocol is also how AsyncAPI distinguishes the two incompatible AMQPs:amqpis 0-9-1, and 1.0 is the separateamqp1protocol. A document declaring no server at all is not an error — plenty describe the contract and leave the broker to deployment, soserversis simply empty.AsyncApiChannelBindings— only the few fields that change what a client has to do are lifted out; everything else stays in a raw map, so nothing is lost and a later transport can read it without the model growing first.The Kafka binding is the one that matters most, because it carries its own
topic, and when present that is the topic a client publishes to rather than the channel address. HenceeffectiveAddress(protocol), which prefers it for Kafka only and leavesaddressuntouched so the document still reads as written. Getting this wrong means publishing to the wrong topic, so it has tests for the override, the non-Kafka case, the no-topic case and the case-insensitive match.Channel parameters, kept verbatim. Resolving a templated address like
tenants/{tenantId}/ordersto a concrete topic is a run-time concern, not a parsing one.AsyncApiSecurityScheme—typeis a free-form string rather than an enum on purpose. AsyncAPI's catalogue is broad and broker-specific (userPassword,scramSha512,gssapi,X509,oauth2, …) and parsing must not fail on a scheme that is perfectly valid but that no transport here can use yet. Whether it can actually be honoured is the connecting client's question. Schemes may also be written inline where they are used rather than incomponents, which real documents do, and those are registered under a name derived from where they appear.Bindings and parameters are dereferenced
Both can be shared through
components, and reading them without following the$refis worse than not reading them at all: a channel whose real Kafka topic sits behind a reference would silently keep its declared address. That is a wrong answer rather than a missing one, so there is a test for it specifically.Testing
71 tests across the package, 13 of them new here.
Fourth in the AsyncAPI stack. Based on #1707.