cylc-flow icon indicating copy to clipboard operation
cylc-flow copied to clipboard

Refactor client/server & GraphQL workflow mutations

Open MetRonnie opened this issue 4 years ago • 2 comments

These changes partially address https://github.com/cylc/cylc-uiserver/issues/271

Sibling: https://github.com/cylc/cylc-uiserver/pull/290

  • Refactored the message format sent from server to client. Instead of just the raw result of the GraphQL mutation (or whatever request was sent[^1]), the server now responds with a tuple of (content, error, user)
    • This allows errors to be returned over the ZMQ socket without mixing/confusing with the data content
    • user is included like this instead of adding it to the GraphQL {data: ..., errors: ...} response (the spec does not permit adding extra keys)
    • A NamedTuple called ResponseTuple is used instead of a regular tuple for better clarity and type safety
  • RefactoredGraphQL workflow mutation output type. Instead of the sometimes-inconsistent GenericResponse output which could be any object in practice, the output is now a graphene.List of GenericReponses, and GenericResponse has 3 fields: workflowId = graphene.String(), success = graphene.Boolean() and message = graphene.String()
    • This pins down what the expected response should be, which should avoid traceback issues like https://github.com/cylc/cylc-uiserver/issues/271
    • In practice this looks like:
      mutation {
        hold (
          workflows: ["temp4"]
          tasks: ["foo.2"]
        ) {
          results {
            workflowId
            success
            message
          }
        }
      }
      
      {
        "data": {
          "hold": {
            "results": [
              {
                "workflowId": "rdutta|temp4",
                "success": true,
                "message": "Command queued"
              }
            ]
          }
        }
      }
      

[^1]: The exception to this is that Protobuf results are sent back as-is in bytes format instead of the tuple, because bytes is not JSON-serializable

Requirements check-list

  • [x] I have read CONTRIBUTING.md and added my name as a Code Contributor.
  • [ ] Contains logically grouped changes (else tidy your branch by rebase).
  • [x] Does not contain off-topic changes (use other PRs for other changes).
  • [x] Applied any dependency changes to both setup.py and conda-environment.yml.
  • [ ] Appropriate tests are included (unit and/or functional).
  • [ ] Appropriate change log entry included.
  • [ ] (master branch) I have opened a documentation PR at cylc/cylc-doc/pull/XXXX.

MetRonnie avatar Nov 23 '21 17:11 MetRonnie

So looks good.. Few minor comments below..

The main issue I'm having (when trying to run this and the UIS sibling), is getting it working

Sorry, should have mentioned: It's not working from the UI yet as some changes need to be made there. But it's working in GraphiQL.

MetRonnie avatar Nov 24 '21 14:11 MetRonnie

Not going to be finished by Christmas so bumping to 8.0rc2

MetRonnie avatar Dec 14 '21 09:12 MetRonnie