Similar looking $ref strings can collide

[romacafe]

Describe the bug
A modular OpenAPI spec can include two $refs with the same relative path, but different targets. These should be treated as different refs, but openapi4j-parser sees them as the same.

To Reproduce
See: https://github.com/romacafe/openapi4j-refs.

Basically, this structure contains identical $ref strings that should refer to different content.

/
+- api.yaml
|  +- $ref testType.yaml#/TestType
|  \- $ref schema2/schema2.yaml#/Schema2
+- testType.yaml
|  \- TestType (definition 1)
\- schema2
   +- schema2.yaml
   |  \- $ref testType.yaml#/TestType
   \- testType.yaml
      \- TestType (definition 2)

Expected behavior
/testType.yaml#/TestType and /schema2/testType.yaml#/TestType should be recognized as different types, even when relative refs (TestType.yaml#/TestType) look the same.

Additional context
Yes this is somewhat contrived, but large APIs are more maintainable when modularized, and it would be good to know conflicts like this can't happen. Otherwise, we'd need to be careful that all $ref values are unique.

[llfbandit]

Welcome and thank you for the neat report!

I'm torn by a potential way to resolve this issue.

To make this issue clearer, the JSON Reference discovering is correct where the registry is not. The getRef(expr) is not sufficient to distinct the two nodes and addRef(baseURI, expr) wrongly flatten the paths.

When adding the 2nd reference to the registry with different base URI compared to the previous, I could throw a ResolutionException but I'm worried about the coverage of this fix and it doesn't feel right...

A better solution is to throw the exception at "OpenAPI level" because you are defining the schema twice (this does not solve the overlapping path for the registry).

Other early solutions I'm thinking about seem heavy and often impossible because there are many cases where we don't know/care about the current positioning.

Such case impacts API description, Schema Object and Operation validations.

This will require investigation before issuing anything...

What do you think about the described solutions (even if it's early)?

[llfbandit]

Are you able to validate this new implementation with the latest snapshot ?

Forget about my previous comment I was wrong when I approached this issue.

You're reproducer won't work without tiny modifications, please see the new behaviour in the merged PR #33 for a complete description of it.

[romacafe]

Excellent! The new approach sounds like it should work, on paper. I will validate today or tomorrow and let you know.

[llfbandit]

The current impplementation introduces a bug with serialization that was not found with current test sets.
The serialization always excludes the new field abs$ref which should not be the case when using TreeUtil.toJsonNode().

This needs to be fixed before closing this issue.

[llfbandit]

It should be back on the right track now. For further info, please see #36.
Can you validate?

[romacafe]

This is not working as I'd expect. It looks like the absolute URI of some references are calculated incorrectly, which makes it impossible to lookup that ref by either the absolute or relative URIs. Also, I think we need a method on Reference to return the new baseURI of whatever the Reference points to, which would be used to follow additional references.

I forked this repo and moved my test case into that fork:
https://github.com/romacafe/openapi4j/blob/romacafe/refs/openapi-parser/src/test/java/org/openapi4j/parser/refs/RefsExample.java

I updated the test with my findings. I'm sure it follows none of your conventions, feel free to modify accordingly.

[llfbandit]

Argh! Thank you eagle eye! Effectively, external refs still can wrongly registered.
Basically, in your setup, schema2 gets an additional path fragment which is leading to something like:

file://boring_path/test/refs/v3/similar/valid/schema2/schema2/schema2.yaml#/Schema2

This is why most of your test results are quite inverted.

Just some comments about RefsExample.java:

• Test section starting at 56: you should use model.getReference(context, $ref) here. But I suppose this was for illustration.
• Test section starting at 70: is now invalidated with the fix. Revert to correctSchema2RelativeReference to pass.

About the method to get the uri of on a Reference, this method already exists: getBaseUri().

Waiting your feedback to validate this critical issue.
Thank you again for your support and your tenacity on this one.

Edit: fix c5ec0f4 has been pushed to master but I forgot to reference this issue.

[romacafe]

Looks good!

you should use model.getReference(context, $ref) here. But I suppose this was for illustration.

Good call. That was not intentional, I just didn't realize there was an easier way. Thanks! Now the test is much simpler:

    OpenApi3Parser parser = new OpenApi3Parser();
    URL url = this.getClass().getResource("/refs/v3/similar/valid/api.yaml");
    OpenApi3 oas = parser.parse(url, false);
    OAIContext context = oas.getContext();

    Schema schema1 = oas.getComponents().getSchema("Schema1");
    Schema testType1 = schema1.getProperty("testType").getReference(context).getMappedContent(Schema.class);
    Schema schema2 = oas.getComponents().getSchema("Schema2").getReference(context).getMappedContent(Schema.class);
    Schema testType2 = schema2.getProperty("testType").getReference(context).getMappedContent(Schema.class);

    assertThat(testType1.getProperty("id").getType(), is("integer"));
    assertThat(testType2.getProperty("id").getType(), is("string"));

or even cleaner:

    OpenApi3Parser parser = new OpenApi3Parser();
    URL url = this.getClass().getResource("/refs/v3/similar/valid/api.yaml");
    OpenApi3 oas = parser.parse(url, false);
    OAIContext context = oas.getContext();

    Schema schema1 = deReference(oas.getComponents().getSchema("Schema1"), context);
    Schema testType1 = deReference(schema1.getProperty("testType"), context);
    Schema schema2 = deReference(oas.getComponents().getSchema("Schema2"), context);
    Schema testType2 = deReference(schema2.getProperty("testType"), context);

    assertThat(testType1.getProperty("id").getType(), is("integer"));
    assertThat(testType2.getProperty("id").getType(), is("string"));

  @SuppressWarnings("unchecked")
  private <T extends AbsRefOpenApiSchema<T>> T deReference(T model, OAIContext context) throws DecodeException {
    if (!model.isRef()) return model;
    return (T) model.getReference(context).getMappedContent(model.getClass());
  }