AsyncAPI 3.x: follow references into other documents - #1707
AsyncAPI 3.x: follow references into other documents#1707LautaroPetaccio wants to merge 8 commits into
Conversation
0d3ff72 to
7c984f4
Compare
7c984f4 to
90989fd
Compare
| */ | ||
| if (absolute.equals(primary)) { | ||
| String fragment = fragmentOf(ref); | ||
| return fragment.trim().isEmpty() ? null : "#" + fragment; |
There was a problem hiding this comment.
replace "#" with constant
There was a problem hiding this comment.
Done! Moved all of the constants to the RefLocation.java file.
| String original, | ||
| List<String> warnings) { | ||
|
|
||
| String path = fragment.startsWith("/") ? fragment.substring(1) : fragment; |
There was a problem hiding this comment.
replace "/" with constant
| @@ -21,4 +28,72 @@ private RefLocations() { | |||
| public static boolean isLocalRef(String ref) { | |||
| return ref.startsWith("#"); | |||
| * @throws IllegalArgumentException if the referring document was supplied as text, as there | ||
| * is then nothing for a relative location to be relative to | ||
| */ | ||
| public static String computeLocation(String ref, DocumentLocation currentSource, List<String> messages) { |
There was a problem hiding this comment.
extractLocation perhaps?
There was a problem hiding this comment.
computeLocation is a bit misleading, the extractLocation is within computeLocation. Find a better way to name this method.
There was a problem hiding this comment.
WDYT about resolveDocumentLocation and extractLocationPart? The second one is now documented to be clearer on what it does.
| try { | ||
| return new URI(location).normalize().toString(); | ||
| } catch (Exception e) { | ||
| return location; |
There was a problem hiding this comment.
what does an exception mean here? Why the location is returned? Add comments to document this decision and behaviour.
There was a problem hiding this comment.
It tried to normalize the path with URI, falling back to the raw string if that failed. But URI rejects paths with spaces or Windows separators, and the raw string was unusable too, so those references were silently dropped.
I've change the code and now a plain path resolves through java.nio.file.Path and a URL through URI.resolve.
90989fd to
c0ed894
Compare
Documents are routinely split across files, so a message payload often points at a schema that lives next door. Until now such a reference could not be followed and the message was dropped. Rather than linking to the other document, its components are copied in: components/schemas and components/messages are merged into the primary document under keys prefixed _ext_<hash>_, and every reference that pointed at them is rewritten to the local form. That keeps the invariant the message layer relies on -- one flat map in which any reference a payload makes can be resolved -- with no second lookup path for whoever consumes a payload later. The hash comes from the absolute location, so the names are stable across runs and generated output stays diffable. Three cases are less obvious, and each has a test: - A reference is resolved against the document that makes it. An imported document referring to 'shared.yaml' means the one next to *itself*. The primary document is therefore rewritten before anything is copied into it: walking it afterwards would re-resolve an imported document's references against the wrong directory and could silently bind them to a different schema that happens to be there. - A document that names itself is not imported into itself. Some generators write every reference as an absolute one, including those that stay inside the file; that is just a local reference written the long way, and is turned back into one rather than doubling every schema in the document. - Only components/schemas and components/messages can be imported. A pointer into some other part of another document is reported and the message depending on it is dropped, rather than left holding a reference that nothing can follow. There is a ceiling of 100 imported documents: references are paths, and a server that answers every path, or a symlink loop, would otherwise be followed forever.
Review feedback: the "#" and "/" that make up a $ref were written as literals wherever they were used. They are now constants on RefLocations, which is the class about reference syntax, and AsyncApiRefResolver reads them from there. Three were pointed out; there were fourteen, so all of them are done rather than only the ones flagged. The same goes for the neighbouring literals in the same expressions -- the protocol separator, the protocol-relative prefix, the http/https prefixes, the parent folder used to resolve a relative location, and the "~0"/"~1" JSON Pointer escapes -- since leaving those as literals would keep exactly the smell being fixed. SCHEMA_PREFIX is now composed from the constants rather than repeating the punctuation, so there is one place where the shape of a component pointer is written down. No behaviour changes: same strings, same comparisons.
| } | ||
|
|
||
| StringBuilder renamed = new StringBuilder("#/") | ||
| .append(COMPONENTS).append('/') |
There was a problem hiding this comment.
replace '/' with constant
| */ | ||
| private static final String PROTOCOL_RELATIVE_PREFIX = "//"; | ||
|
|
||
| private static final String HTTP_PREFIX = "http" + PROTOCOL_SEPARATOR; |
There was a problem hiding this comment.
replace "http" with constants
c0ed894 to
bd15770
Compare
…tp check Follow-up to the previous commit, for what it left behind. - "http" and "https" are constants, so the prefixes are built from a named scheme rather than from a literal. - The pointer rebuilt in renameComponent spelled its own punctuation out. It now uses the constants, through a new COMPONENT_PREFIX that says once what "#/components/" is; SCHEMA_PREFIX is built from that too, so the shape of a component pointer is written down in one place. The warning beside it no longer hardcodes "components/" either. - schemaKeyOf still looked for a literal '/'. The check for an absolute http(s) location existed twice, once here and once in the resolver, spelled out both times. It is now RefLocations.isHttpLocation, which removes the duplicated logic rather than just naming the literals in each copy. No behaviour changes.
Review feedback: computeLocation and extractLocation read as equals, so seeing one nested inside the other suggests the containment is the wrong way round. "Compute" and "extract" are near-synonyms here and say nothing about which does more. - computeLocation -> resolveDocumentLocation - extractLocation -> extractLocationPart The verb now carries the difference: one extracts a part of the reference text, the other resolves that part against the document making the reference, following relative paths and borrowing protocols. Naming what is extracted also removes the clash, since it is the location *part* of a reference rather than a location in its own right. extractLocationPart gains the javadoc it never had. Both are verb-first, which is what the rest of the repository does: 85% of methods in arazzo-parser and dbconstraint, and 77% in core. Renaming diverges from SchemaUtils on the OpenAPI side, which still has the original pair. That seems the right trade: this module is standalone and meant to move out of EvoMaster, so its own clarity matters more than matching code it will never share.
…s its location Review feedback asked what the exception around URI.normalize() meant and why the location was returned anyway. Looking into it, the honest answer was that it meant the reference was about to be dropped. A relative location was resolved by building "<document>/../<target>" and normalizing that with java.net.URI. Two problems: - URI rejects anything that is not a legal URI. A path containing a space is not, and neither is a Windows path, with its backslashes and drive letter. - The fallback then returned the string un-normalized, which is not usable either: ".." cannot traverse through a file, so the path does not exist. So every external reference was dropped, with a warning saying the file was not found, for any project sitting in a folder whose name contains a space -- and, by the same route, on Windows. It degraded rather than failing loudly, which is why it went unnoticed. The fix dispatches on what kind of location the referring document has, which the caller already knows, and lets the JDK resolver for that kind do the work: - a URL, a file: URL, or a classpath path is resolved as a URI, per RFC 3986, which defines what a trailing slash means and collapses "." and ".." segments -- so two references to the same document produce the same string, which matters because that string is what tells imported documents apart; - a plain file path is resolved through java.nio.file.Path, which knows the platform's separator and accepts a space or a backslash without complaint. Hand-rolling either set of rules is what caused this in the first place. A URL that is itself not a valid URI, such as one written with a space, now reports that a relative reference cannot be resolved from it, rather than guessing; there is no well-defined answer in that case. Covered by tests: a folder with a space in its name end to end, which fails without the fix; and the resolution rules for ".", "..", a directory URL and a file: URL, plus the reported case.
The history belongs in the commit that made the change, not in the code.
Whether a LOCAL location is a plain file path or a file: URL decides how a relative reference is resolved against it, and the resolver was working that out inline from the type plus a prefix check. DocumentLocation now answers it through isPlainFilePath(), so the distinction has one home and the resolver reads as a single decision.
Measured with jacoco, the resolution path was well covered but several real behaviours around it had never run in a test: - importing a document from a remote location. AsyncApiDocumentFetcher is an interface precisely so this can be tested without a server, and no test had used it. One now stubs it and asserts what was asked for and how. - the ceiling of a hundred imported documents, and its warning. - a reference to a whole document rather than to a component of it. - a reference with no fragment at all, which must not even be fetched. - percent-encoded pointer segments, including one that cannot be decoded and is taken as written. - a plain http absolute reference; everything before used https. DocumentLocation gets a suite of its own. Its equals and hashCode had no production caller and no test, which for a value type is a trap waiting for the first use as a key; and isPlainFilePath is checked against every kind of location it has to tell apart. RefLocations and DocumentLocation are now at 100% of lines and branches; AsyncApiRefResolver goes from 88% to 92% of branches. What remains there is either unreachable by design (the SHA-1 lookup cannot fail) or reachable only through exotic input (an illegal path character), and is left alone.
Fourth in the AsyncAPI stack, on top of #2.
Documents are routinely split across files — a message payload points at a schema that lives next door. Until now such a reference could not be followed, and the message was dropped.
Inlined, not linked
Rather than keeping a second lookup path for "schemas that live elsewhere", the other document's
components/schemasandcomponents/messagesare copied into the primary one under keys prefixed_ext_<hash>_, and every reference that pointed at them is rewritten to the local form.That preserves the invariant the message layer rests on: one flat map in which any reference a payload makes can be resolved. Whoever builds genes later still only has to look in
componentSchemas.The hash is derived from the absolute location, so the names are stable across runs and generated output stays diffable.
Three cases that are easy to get wrong
A reference is resolved against the document that makes it. An imported document referring to
shared.yamlmeans the one next to itself, not next to the primary document. So the primary document is rewritten before anything is copied into it — walking it afterwards would re-resolve an imported document's relative references against the wrong directory, and if that happened to hit another loaded document it would bind them to a completely different schema with no sign anything was wrong.nested/is a fixture built to reproduce exactly that, and the test asserts the reference is left unresolved and reported rather than silently misbound.A document that names itself is not imported into itself. Some generators write every reference as an absolute one, including those that stay inside the file.
self-reference.yaml#/components/schemas/Thingfrom insideself-reference.yamlis just a local reference written the long way; it is turned back into one, rather than importing the whole file into itself and doubling every schema and message it declares.Only
components/schemasandcomponents/messagescan be imported. A pointer into some other part of another document — an OpenAPIx-webhookssection, say, which is a real case — is reported, and the message depending on it is dropped rather than left holding a reference that nothing downstream can follow.Bounded
There is a ceiling of 100 imported documents. References are paths, and a server that answers every path, or a symlink loop, would otherwise be followed forever.