Uninformative error on self-conflicting types

[tormeh]

I used the following OpenAPI schema:

{
  "components": {
    "schemas": {
      "Foo": {
        "allOf": [
          {
            "$ref": "#/components/schemas/Bar"
          },
          {
            "properties": {
              "value": {
                "type": "string"
              }
            }
          }
        ],
        "required": [
          "value"
        ]
      },
      "Bar": {
        "properties": {
          "value": {
            "type": "object"
          }
        }
      }
    }
  },
  "info": {
    "title": "",
    "version": "1.0"
  },
  "openapi": "3.0.1",
  "paths": {
  }
}

I expected an error saying that the type of value in Foo was ambiguous. I got:

thread 'main' panicked at /Users/[my_user]/.cargo/registry/src/index.crates.io-6f17d22bba15001f/typify-impl-0.0.15/src/merge.rs:265:70:
called `Result::unwrap()` on an `Err` value: ()

It seems silly in this example, but the original schema was 10000 lines long, so it was quite tedious to isolate the issue.

Meta

Progenitor version used was 0.5.0.
I used a build.rs file to invoke progenitor.

[tormeh]

Btw, some other OpenAPI codegens resolve the conflict between object and string to string, which may or may not be reasonable. Maybe this can be a default-off resolution strategy in progenitor?

[jclulow]

Thank you for the isolated reproduction, and sorry about the unhelpful nature of the message there.

This does indeed seem like a conflict -- what does it say in the prose documentation for the part of the API that the broken schema describes? That is: are they actually expecting a string or an object?

[tormeh]

The spec and the documentation is autogenerated based on annotations in some Java code that I'm not very familiar with. Bar is a generic abstract class, and in Bar<T> value has type T. In this example Foo extends Bar<String>. There are other classes that extend Bar<Baz>, which explains how it ended up as object in the OpenAPI schema.

[tormeh]

It's important to note, though, that only Java classes extending Bar<String> are components in the OpenAPI schema. So a sophisticated code generator may see that Bar is never used on its own, and in every type conflict between Bar and something else (X), X.value is always string. It's therefore safe to change Bar.value from object to string. I don't think it's reasonable to expect a code generator to do this, though. The OpenAPI schema has a bug in it, and it's probably preferable that the code generator raises an error rather than silently papering over the bug.

[ahl]

I've moved this to the typify repo since the code it pertains to lives here. Progenitor hands of processing of types to typify.

First, yes, the error message need to be improved. There's ongoing work to maintain context so that, in a case such as this one, the error pointed you to the specific part of the large specification document where the problem was encountered.

It seems like the schema for Bar is wrong for its intended use. Perhaps it should be something like:

      "Bar": {
        "properties": {
          "value": {}
        }
      }

or (equivalently):

      "Bar": {
        "properties": {
          "value": true
        }
      }

I'm not sure what weight to put on the fact that Bar isn't used directly. In general, typify (and progenitor) studiously avoid heuristic handling of potentially misconstructed schemas. Specifically, when a schema is innately unsatisfiable, there is no intrinsically correct answer regarding which constraints one might relax to resolve that unsatisfiability. In fact, the direction we've been going is to use "never" types in situations such as these (enum Never {} i.e. an enum with no variants that is therefore uninstantiatable).

My general recommendation has been to apply a JSON patch to the input document before passing it to progenitor (or to typify).

[tormeh]

I agree. My solution for now is to preprocess the schema in build.rs, as it is the schema that is wrong.

I don't think a lot of context is necessary, although I wouldn't exactly complain about a rustc-style error message with line numbers and everything. It might be enough to state the name of the type that couldn't be determined. Just knowing that Foo.value was the issue would have helped a lot, though the error would ideally say Foo and Bar conflicted with regards to Foo.value.

[ahl]

I don't think a lot of context is necessary, although I wouldn't exactly complain about a rustc-style error message with line numbers and everything. It might be enough to state the name of the type that couldn't be determined. Just knowing that Foo.value was the issue would have helped a lot, though the error would ideally say Foo and Bar conflicted with regards to Foo.value.

Absolutely: I really just want to give you a json path since some specs don't contain newlines so saying "somewhere on line 1" wouldn't be helpful.

[tormeh]

I think #521 might also be somewhat useful here.