Cannot easily define server errors generic to all methods

BelfordZ commented 5 years ago

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] commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

@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 commented 4 years ago

To summarize the proposed solution:

we would be adding a new field to components called ErrorGroups
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 commented 4 years ago

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 commented 4 years ago

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

peol commented 4 years ago

@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 commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

@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 commented 4 years ago

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 commented 4 years ago

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

BelfordZ commented 4 years ago

open-rpc / spec

Cannot easily define server errors generic to all methods #225