Allow adding refs from pre-processors

[konstantin-smolyakov-idt]

Is your feature request related to a problem? Please describe. I would like to implement a custom plugin to conventionally add responses to each operation. For example, I would like to add 400 errors to all operations in my spec without having to specify it each time.

Describe the solution you'd like

Let's imagine a configuration file with the following entries. It will mean that all operations can return 400 or 422. Potentially, this can be extended with a filter to include only certain operations by convention (e.g. a delete request might not return 400 error).

preprocessors:
    my-custom-pluging/conventional-response:
      '422':
        response:
          $ref: './common/errors/validation-error.response.yaml'
      '400':
        response:
          $ref: './common/errors/bad-request-error.response.yaml'

I've tried to implement a PoC for this, but it appears that references are all processed before any plugins are invoked. So I end up with Can't resolve $ref error messages from the bundler.

Describe alternatives you've considered The alternative will be to build a rule to enforce this. But this is not the same, because there is no way to auto-fix the issue and it adds some churn on the developer to remember to add all standard errors to operations.

Additional context I understand that this might not go well with the idea of reporting the precise error location in case other validators run on post-processed results. However, maybe it can be solved by restoring the post-processed tree somewhere in the temporary folder and pointing issues there.

[adamaltman]

Have you considered writing a decorator to do this? I was thinking of a similar thing (common headers) recently (but as a decorator).

[adamaltman]

Also, I suppose you could have these in your components, and reference the components? For example:

components:
  responses:
    validation-error:

And then you would inject $ref: #/components/responses/validation-error instead.

[RomanHotsiy]

I understand that this might not go well with the idea of reporting the precise error location in case other validators run on post-processed results

Yeah, preprocessors will break error reporting right now and this is not too simple to resolve the issue so we try to avoid them. They will also break autocompletion in our vs code plugin, etc.

We do in fact run "resolve" step before any preprocessor takes in to improve performance and to make our rules run synchronously. I believe we used to run resolve again after preprocessors run but looks it was removed, I think we can re-add it.

On the other hand, while I understand you want to reduce the duplication, note that you will lose some transparency.

[konstantin-smolyakov-idt]

Have you considered writing a decorator to do this? I was thinking of a similar thing (common headers) recently (but as a decorator).

I thought about this. However, it means that my referenced schemas are never validated also. If they are not included explicitly anywhere that it means they are unreferenced for the compiler.

Also, I suppose you could have these in your components, and reference the components?

This might be a good workaround, didn't consider it. However, I prefer to keep all the schemas in separate files. I ended up with writing a validation rule instead as an alternative I mentioned before. It's not ideal but it helps to keep consistency I try to achieve.

On the other hand, while I understand you want to reduce the duplication, note that you will lose some transparency.

That's why I'm suggesting to expose post-processed result somehow for the end-user. For example it could stay in some temporary directory not committed into the source control: either in a bundled form or copying the original directory tree. Then the end-user can:

• See the post-processed result with his own eyes (be it a decorator or a pre-processor). I would even say that the decorators are not needed if validators are always run on the end-result.
• Navigate from error messages to this temp location to pinpoint the problem.

I understand that this might be not possible as it's kind of breaks how the core of this product works. Also it's a downside as errors are not reported in an original location (not how traditional code works). I guess it's a common trade-off between configuration vs code problem. E.g. see Terraform vs Pulumi approach (DSL vs programming language). Eventually, engineers tend to make configuration so flexible that it almost becomes the programming language. So maybe we should just describe OpenAPIs using TypeScript functions returning OpenAPI objects 😄

autocompletion in our vs code plugin, etc.

Didn't know it exists, will check it out 👍

[skilbjo]

Hope you don't mind me asking a somewhat related/unrelated question,

@konstantin-smolyakov-idt brought up an interesting observation that I didn't find in the docs,

it appears that references are all processed before any plugins are invoked. So I end up with Can't resolve $ref error messages from the bundler

I was potentially under the impression that $refs weren't resolved before preprocessors ran, however, I am encountering a similar issue. (Probably similar to how @konstantin-smolyakov-idt 's POC worked)

(From https://redoc.ly/docs/cli/resources/custom-rules/ )

Isn't this how the old transformers used to work ($refs weren't resolved before preprocessors ran)? In the old transformers, those plugins ran before $refs were resolved?

Or is it more as @RomanHotsiy said,

We do in fact run "resolve" step before any preprocessor takes in to improve performance and to make our rules run synchronously. I believe we used to run resolve again after preprocessors run but looks it was removed, I think we can re-add it.

And once the resolve() step is run again after preprocessors run, my & @konstantin-smolyakov-idt 's POC should work?

?

[skilbjo]

Dug in to the the @redocly/openapi-cli@0.2.16 code a bit (difficult because there isn't a tag, I just am able to grab the npm package, but check this out:

  const resolvedDefinition = (0, _resolveDefinition.default)(definition, ctx, nodeContext.resolvedNode);
  ctx.definitionStack.push(resolvedDefinition);
  (0, _scalarsResolver.default)(nodeContext.resolvedNode, definition, ctx);

  if (definition.customResolveFields) {
    await definition.customResolveFields(nodeContext.resolvedNode, ctx, visited);
  }

@redocly/openapi-cli@0.2.16/dist/traverse.js

it looks like it is resolving $refs twice no? whereas I don't see any such corresponding code in the 1.0.x releases

[RomanHotsiy]

@skilbjo yes. We disabled it for performance reasons mainly and also because transformers are tricky.

E.g. we can't report error location correctly after transformer run.

I think we can enable it back using some flag like --resolve-after-transformers. What do you think?

[skilbjo]

@RomanHotsiy thanks for the answer, that would be great. I would appreciate that.

I ended up using a really poor hack, in my package.json,

scripts: {
  ...
  "prebundle": "openapi bundle -o src/openapi.preprocessed.json spec/openapi.yaml --force --config .redocly.pre-processor.yaml 2>/dev/null && echo 'warning: errors from pre-processing step have been supressed'",
  "bundle": "openapi bundle -o src/openapi.json src/openapi.preprocessed.json
  ...
}

and keep two .redocly.yaml configs, but I am not a fan of this... having a flag to re-run resolve again after pre-processing would be wonderful

[konstantin-smolyakov-idt]

I would also say that running validations on a fully bundled specification is also easier. E.g. when writing custom rules, it's easier to deal with local $refs only. Similarly to @skilbjo I was thinking about a 2-step process:

• Bundling the specification with custom decorators to augment it. Produce an intermediate OpenAPI file.
• Run redocly again to validate that final file. It'll make sure that validation rules address both the YAML added by decorators and YAML written manually. Validation error pointers are also good because you can see that intermediate bundled file.

It's not perfect, but this model fits in my head better as what we want is to get a finally valid spec after all our modifications (both manual and automatic).

[RomanHotsiy]

Bundling the specification with custom decorators to augment it. Produce an intermediate OpenAPI file.

This makes error location detection almost impossible (or way to complicated).

[TylerRick]

I'm running into this as well. I added a preprocessor/decator (tried both ways) that automatically adds common responses (400, 401, 403) to all operations so that I don't have to manually add it to each operation. But bundle fails with a whole bunch of Can't resolve $ref errors.

I tried changing the $ref to both how it is in the source file ('../components/responses/bad_request.yaml') and then tried how it is in the bundle file ('#/components/responses/bad_request') and it doesn't like either way.

It looked like many of the $refs that it was complaining about were not the ones added by the preprocessor, but seemed to be ones that were already there, which I found amply confusing, and eventually gave up trying to troubleshoot and figure out.

Since I can't fix it, how about a way to silence it? It said Error was generated by the bundler rule. so I tried to silence it with bundler: off in config as well as with --skip-rule bundler — both with no success — which I find confusing since it says the rule is called bundler, so one might assume it is also skippable/turn-offable by that time.

Fortunately, I did finally find a way to silence the errors and make it bundle it anyway using --max-problems 0 --force.

That is less than ideal, obviously, because I would like to see any problems (other than the 95 seemingly spurious Can't resolve $ref errors, that is).

The output when using --force and '#/components/responses/bad_request'-style references (as long as I have at least 1 operation where the ref was manually added, to trick it into actually including that $refed component in the generated output) does seem to be correct/valid/usable, however, so I'm continuing to use this approach for now.

Needless to say, it would be interested in having a quieter, cleaner (no --force) solution or workaround to this problem/limitation of decorators — even just a cleaner way to silence the errors would be appreciated — Or, too, a completely different, blessed way to add common non-unique responses without duplicating them in every operation...

[lornajane]

After chatting more about this @RomanHotsiy would this need changes to the existing behaviour, or adding a new flag with new capabilities later? I think we should do it, just from a project point of view would it need to wait for a major version release or could we add it in a minor version?

[RomanHotsiy]

I think we should just run another pass of "resolve" after preprocessors. Adding common responses is a pretty valid use case which I would love to support.

We should add a note about the undefined behaviour with error reporting.

This requires a small change in the core.

[tatomyr]

I'm not sure it will work predictably even if we run "resolve" again after preprocessors. The thing is, the refs are relative to redocly.yaml (which is supposed to be at the root), while the definition root might be located in a nested folder. Thus links that are valid for one file won't work for another. If we want to allow using $refs in the config, we have to resolve redocly.yaml itself. And only then apply preprocessors.

[RomanHotsiy]

The thing is, the refs are relative to redocly.yaml (which is supposed to be at the root)

I think you're misunderstanding something. I don't think this is the case. Let's discuss it on the call.

[rotorgames]

@tatomyr Hey. I'm working on the custom $ref resolvers that allows to use variables or key-words in the $ref path, Unfortunately, seems all preprocessors run after resolving $refs for now, so I can't update the $ref with a new path and location in the preprocessor.

However, I can see that you started working on the solution around that issue. So, could you share any ETA of that and a pathway about how are you going to fix it (second resolve call or something else?)

Thanks.

[tatomyr]

@rotorgames I'm working on this on and off. Hopefully will get my hands on it this week. Yes, I expect resolving twice to be the solution.