Cannot easily define server errors generic to all methods

[BelfordZ]

At the moment, if you have server-defined errors that can be returned by any of the methods, you would have to $ref the error(s) in each method you have.

This amounts to a lot of repetition.

[stale[bot]]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

[peol]

Hi. I see this was queued to be discussed, has there been any decisions how this should be handled? We're facing this issue currently and it's not feasible for us to inline 20+ errors in 100+ methods unfortunately. If we can help with e.g. a proposal, would that be interesting to you?

[shanejonas]

Hi, thanks for commenting with your issue, I think the proposal would be something close to the language around servers and servers.method, except it would be joining instead of overriding the errors that are on the method.

Another short term remedy is to use $refs to at least provide minimal code re-use:

        "errors": [
          {
            "$ref": "#/components/errors/bar"
          },
          {
            "$ref": "#/components/errors/foo"
          }
        ],

Some addition thoughts on this:

Error Groups:

If there was a way to combine groups of errors, we wouldn't need to have a root errors, we could just combine them. @zcstarr can maybe elaborate on this

[zcstarr]

My thought here is that perhaps we don't need a top level default errors field. If we make errors composable, where we can create error groupings. Then for each method you'd be able

"method": ...,
"errors": [
          {
            "$ref": "#/errors/DefaultErrorGroup"
          },
          {
            "$ref": "#/components/errors/customMethodError"
          }
        ],

This would then allow us to be specific vs. having a hierarchical precedence for errors. I think with configuration and specification having scenarios where parts of the configuration are overridden adds a lot of cognitive load. I.e. at first glance you it becomes hard to figure out what's happening.

I'm curious if this scenario would work for you. I think it would in your case it would be 20 methods that are then updated with a single default ErrorGroup that you would be able to then append custom method specific errors to when needed. Updating would then involve updating the errorGroup. This would also allow you to opt out of certain methods implementing the error group.

What do you think?

[peol]

Thank you for re-opening the issue and the quick proposal! I think this is a good approach and actually exactly how I tried to solve it before noticing the spec wouldn't allow it :)

👍

[BelfordZ]

@peol thank you for bringing it back up.

We try not to solve problems no one is having, so this really helps.

Im happy to make a PR adding this, and Im also happy to help you in putting one together if thats something you are interesting in doing. It should be fairly straight forward, you can find instructions if needed in the CONTRIBUTING.md

[BelfordZ]

To summarize the proposed solution:

1. we would be adding a new field to components called ErrorGroups
2. method.errors accepts an array of errors, which can be intermixed with ErrorGroups, where the resulting error set, once normalized, yields the union of all errors provided.

[peol]

Thank you @BelfordZ — I got to be honest, I'm not super savvy at schemas so if it's not too much trouble for you I'd appreciate your help here. Let me know if you do not have the bandwidth right now and I'll take a swing at it, of course!

[BelfordZ]

Got something started @peol maybe you want to take a look?

[peol]

@BelfordZ Awesome, definitely! I'll dig into it and verify with our use case next week when I am back, and then get back to you if that sounds OK?

Happy holidays!

[peol]

Hi again @BelfordZ — apologies for dragging this out, I never got around to this during the Christmas break.

I've tried it out internally, and if this output looks correct, it works like a charm (brain's still a bit mushy from holidays, so I might've misinterpreted something):

{
  "openrpc": "1.0.0",
  "info": {
    "title": "...",
    "version": "12.542.1-dev..."
  },
  "methods": [
   {
      "name": "Field.GetCardinal",
      "description": "Retrieves the number of distinct values in a field.",
      "params": [],
      "result": {
        "name": "Response object",
        "description": "This object contains dynamic properties returned by a successful method call",
        "schema": {
          "type": "object",
          "properties": {
            "qReturn": {
              "type": "integer"
            }
          }
        }
      },
      "errors": [
        {
          "$ref": "#/components/errorGroups/GenericErrors"
        }
      ]
    }
  ],
  "components": {
    "schemas": {},
    "errors:": {
      "LOCERR_INTERNAL_ERROR": {
        "code": -128,
        "message": "Internal error"
      },
      "LOCERR_GENERIC_UNKNOWN": {
        "code": -1,
        "message": "Unknown error"
      }
    },
    "errorGroups": {
      "GenericErrors": [
        {
          "$ref": "#/components/errors/LOCERR_INTERNAL_ERROR"
        },
        {
          "$ref": "#/components/errors/LOCERR_GENERIC_UNKNOWN"
        }
      ]
    }
  }
}

[shanejonas]

playing around with this today i think we can do it without introducing an errorGroups or groups concept by using oneOf

heres an example that atleast validates, not sure how the tooling handles it:

{
  "openrpc": "1.0.0-rc1",
  "info": {
    "version": "1.0.0",
    "title": "Petstore Expanded"
  },
  "methods": [
    {
      "name": "get_pets",
      "params": [
        {
          "name": "tags",
          "description": "tags to filter by",
          "schema": {
            "type": "array",
            "items": {
              "type": "string"
            }
          }
        },
        {
          "name": "limit",
          "schema": {
            "type": "integer"
          }
        }
      ],
      "result": {
        "name": "pet",
        "schema": {
          "type": "array",
          "items": {
            "$ref": "#/components/schemas/Pet"
          }
        }
      },
      "errors": [
        {
          "$ref": "#/components/errors/GenericErrorGroup"
        }
      ]
    }
  ],
  "components": {
    "errors": {
      "LOCERR_INTERNAL_ERROR": {
        "code": -128,
        "message": "Internal error"
      },
      "LOCERR_GENERIC_UNKNOWN": {
        "code": -1,
        "message": "Unknown error"
      },
      "GenericErrorGroup": {
        "oneOf": [
          {
            "$ref": "#/components/errors/LOCERR_INTERNAL_ERROR"
          },
          {
            "$ref": "#/components/errors/LOCERR_GENERIC_UNKNOWN"
          }
        ]
      }
    },
    "schemas": {
      "Pet": {
        "allOf": [
          {
            "$ref": "#/components/schemas/NewPet"
          },
          {
            "required": [
              "id"
            ],
            "properties": {
              "id": {
                "type": "integer"
              }
            }
          }
        ]
      },
      "NewPet": {
        "type": "object",
        "required": [
          "name"
        ],
        "properties": {
          "name": {
            "type": "string"
          },
          "tag": {
            "type": "string"
          }
        }
      }
    }
  }
}

[peol]

This would work for us as well, of course. It does look a bit more elegant. I'm happy to update my PR (https://github.com/open-rpc/meta-schema/pull/170) once you've got a decision on this issue :) Thanks for following up!

[peol]

@BelfordZ @shanejonas Hi fellas, just following up on this. Are you OK with committing to the new proposal above? If so, I can get going on a PR to reflect this in the meta-schema. 👨‍💻

[BelfordZ]

All good . I too have been dragging my heals with this one.

Of proposed solutions, I am leaning towards @shanejonas 's suggestion above. Let's leave this to be found by others for a couple more days since taking things out is infinitely harder than putting them in.

RFC @zcstarr @meowsbits @AArnott

[AArnott]

I'm not going to have time to brainstorm on this one, sorry.

[BelfordZ]

Similar problem, different solution:

https://github.com/OAI/OpenAPI-Specification/issues/417

[BelfordZ]

What do you think about implementing your prefered solution simply as a script that preprocesses your document? This way you can try out the suggested feature before we buy it ;)

[peol]

@BelfordZ Sorry about the slow response, been busy with the adjusted work situation with covid-19 and all that... I'm sure you are too. I don't exactly follow what you mean with 'a script that preprocesses your document'. Do you mean for testing purposes? We generate our spec in runtime so it's almost like that today, I just haven't had time to follow up until now. Using oneOf works great from what I can tell, granted that the schema allows it (today, an error cannot be defined as a list of oneOf I believe).

Keep safe!

[stale[bot]]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

[simsonraj]

Hey @BelfordZ, Wanted to follow up on this issue. We are working on a similar spec definition that needs provision to define errors and reuse them in multiple methods. redefining errors for each method will complicate the spec. OneOf will work for us or at least the ability to define a range of numbers for the error code, since as per JSON-RPC spec it leaves a big range to be defined as per application usage. I see this related PR crossed out the proposed error grouping or oneOf changes. Any chance of merging this?

[zcstarr]

@shanejonas and I were talking about this today, and we think a solution that might be an easier and cleaner option, that might be better to support the ecosystem by just extending errors to support json pointers. @shanejonas has some more thoughts he'll drop here later