Update camunda REST API doc #4308
Conversation
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
github-actions
bot
force-pushed
the
update-rest-doc
branch
from
34228bf
to
7d39526
Compare
| decisionId: | ||
| description: | | ||
| The ID of the decision to be evaluated. | ||
| Cannot be used together with decisionKey. When using the decision ID, the latest |
There was a problem hiding this comment.
@tmetzke: @jwulf brought up in Slack that a similar endpoint, in which the request can contain either an ID or a key but not both, was resulting in confusing docs, because the example request included both an ID and a key.
There are a couple things we should do in our spec to account for these types of situations. I'm bringing them up here to start a conversation, though I will merge this PR as-is, and we can handle any resolutions outside of this. I'm interested to know how you feel about these suggestions, who you think should own a PR to update this specific spec, and how you think we can communicate this practice to the engineers developing specs.
The proposed changes
1. Spec union types more intentionally
The EvaluateDecisionRequest can be spec'd such that the schema eliminates the possibility of including both an ID and a key. I think the correct way to do this is with the oneOf union, though I am no OpenAPI expert and if others know of a better mechanism, I defer to them 😅. Here's an example of how we can write this request object so that it can only include an ID or a key:
EvaluateDecisionRequest:
type: object
oneOf:
- $ref: '#/components/schemas/EvaluateDecisionRequestKey'
- $ref: '#/components/schemas/EvaluateDecisionRequestId'
EvaluateDecisionRequestKey:
type: object
allOf:
- $ref: '#/components/schemas/EvaluateDecisionRequestBase'
properties:
decisionKey:
description: |
The unique key identifying the decision to be evaluated.
Cannot be used together with decisionId.
type: integer
format: int64
EvaluateDecisionRequestId:
type: object
allOf:
- $ref: '#/components/schemas/EvaluateDecisionRequestBase'
properties:
decisionId:
description: |
The ID of the decision to be evaluated.
Cannot be used together with decisionKey. When using the decision ID, the latest
deployed version of the decision is used.
type: string
EvaluateDecisionRequestBase:
type: object
properties:
variables:
description: The message variables as JSON document.
additionalProperties: true
type: object
tenantId:
description: The tenant ID of the decision.
type: stringHere's how that schema gets rendered by Swagger:
And here's how it would get rendered by our docs:
2. Add example requests
As it currently stands, the example request in the docs is improperly formed -- it contains both an ID and a key. Addressing item 1 above would resolve this, in that the docs will generate a properly formed request given the union type described above.
But we can make this even better, by providing example requests for the different possibilities. Here's an example modification to the endpoint request definition, so that two examples are provided:
/decisions/evaluation:
post:
tags:
- Decision
summary: Evaluate decision
description: |
Evaluates a decision.
You specify the decision to evaluate either by using its unique key (as returned by
DeployResource), or using the decision ID. When using the decision ID, the latest deployed
version of the decision is used.
requestBody:
required: true
content:
application/json:
schema:
$ref: "#/components/schemas/EvaluateDecisionRequest"
examples:
"Decision Key":
summary: "Identify the decision by key"
value:
decisionKey: 123
variables: { }
tenantId: 987
"Decision ID":
summary: "Identify the decision by ID"
value:
decisionId: "1234-5678"
variables: { }
tenantId: 987
...Here's how the examples appear in Swagger UI:
And here's how they'd appear in our docs:
There was a problem hiding this comment.
Both of these - strict grammar and meaningful representative example data - will make a huge difference.
There was a problem hiding this comment.
Thanks for bringing this up and great proposal, @pepopowitz 👍
One aspect of the oneOf mechanism that OpenAPI provides is that it poses an issue with the Java models we generate from it. There are multiple issues with this polymorphism mechanism in the code generators and not only for Java, if I remember correctly. The generators struggle to create code that works in an API setup where data needs to be (de-)serialized, leading to custom post-processing effort required by generator users (us, in this case) to make it work. I would like to avoid introducing this risk for this and other endpoints, especially now, since the code freeze for 8.6 is next Monday 😅
Thus, I propose to go the pragmatic way of adding proper examples to the spec and leaving the endpoint structurally as it is.
Let me know if that makes sense or if I missed anything.
There was a problem hiding this comment.
Thanks for the clarification! That makes sense, and I agree about avoiding last-second risk (and the freeze was on already by the time I responded anyway 😅).
-
It sounds like we're unanimous about adding examples to the spec. Since the freeze is on, I'll do two things: (a) open a PR against the camunda repo with the examples, knowing that it won't be merged until after the freeze, and (b) open a PR against this repo to add a step to our generation strategy in the docs, to apply the examples when generating the docs.
-
I don't feel great about introducing drift, but is this a situation where we might want the spec to drift between the two repos? The docs benefit in multiple ways from the
oneOfdefinition (an actually-working default example, more clear schema). We already have the ability to introduce custom changes to the spec when generating the docs, and could take advantage of that. Does anyone think it would cause recurring difficulties if we left the Java-friendly definition alone in the camunda repo, but swapped in the docs-friendly definition in the camunda-docs repo? If not, I can PR this. In fact, I think I'll open the PR regardless, and we can have more conversation there about the risks/rewards it introduces.
There was a problem hiding this comment.
Does anyone think it would cause recurring difficulties if we left the Java-friendly definition alone in the camunda repo, but swapped in the docs-friendly definition in the camunda-docs repo? If not, I can PR this. In fact, I think I'll open the PR regardless, and we can have more conversation there about the risks/rewards it introduces.
Thanks for opening this conversation @pepopowitz. Can you bring this to ask-developer-experience so we can chat more on Slack?
There was a problem hiding this comment.
Temporary example requests are added on the docs side at #4352.
|
👋 🤖 🤔 Hello! Did you make your changes in all the right places? These files were changed only in docs/. You might want to duplicate these changes in versioned_docs/version-8.5/.
You may have done this intentionally, but we wanted to point it out in case you didn't. You can read more about the versioning within our docs in our documentation guidelines. |
|
The preview environment relating to the commit 07eef3d has successfully been deployed. You can access it at https://preview.docs.camunda.cloud/pr-4308/index.html |
… request fields (#22711) ## Description Adds valid example requests to the C8 API spec for two endpoints, each of which accepts mutually exclusive fields in requests: * POST /process-instances * POST /decisions/evaluation ## Checklist <!--- Please delete options that are not relevant. Boxes should be checked by reviewer. --> - [ ] for CI changes: - [ ] structural/foundational changes signed off by [CI DRI](https://github.com/cmur2) - [ ] [ci.yml](https://github.com/camunda/camunda/blob/main/.github/workflows/ci.yml) modifications comply with ["Unified CI" requirements](https://github.com/camunda/camunda/wiki/CI-&-Automation#workflow-inclusion-criteria) ## Related issues Original issue was discussed in camunda/camunda-docs#4308 (comment).
Description
This is an autogenerated PR by the sync api specs workflow.
This PR contains every changes made to the REST api specs in the monorepo in the last week.
This PR contains also all the generated OpenAPI files related to the changes.
When should this change go live?
PR Checklist
/docsdirectory (aka/next/).