Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use query fragments instead of strings to merge/combine queries #439

Closed
kinow opened this issue Apr 2, 2020 · 10 comments · Fixed by #726
Closed

Use query fragments instead of strings to merge/combine queries #439

kinow opened this issue Apr 2, 2020 · 10 comments · Fixed by #726
Assignees
Milestone

Comments

@kinow
Copy link
Member

kinow commented Apr 2, 2020

Describe exactly what you would like to see in an upcoming release

Investigate using graphql-tag to merge queries.

That would allow us to use any sorting keys (#300), use variables (#234), and use graphql-tag imports instead of strings (#356).

And maybe with more work we can fix #209 and #417 too.

Additional context

i. Queries will have the same operation type subscription, and will/must share the root level field. The root level will be workflow. For example:

subscription A {
  workflow { id }
}

subscription B {
  workflow { nodesEdges { nodes { id } } }
}

subscription AB {
  workflow {
    id
    nodesEdges {
      nodes {
        id
      }
    }
  }
}

ii. Variables must match. Meaning, too, that we are not merging a query like GScan's, that doesn't have any variables, with a query that is limited for a single workflow like the Tree view query. So the following example will result in an error.

subscription TreeView ($workflowID: ID) {
  workflows (ids: [$workflowID]) {
    id
    jobs { id }
  }
}

subscription GScan {
  workflows {
    host
    port
  }
}

// Otherwise if we had 500 workflows in Cylc, that would result in the query above returning the host+port **and** the jobs for all the 500 workflows; which was a bug in the previous implementation
// We want the 500 workflows for GScan to display int he sidebar, but only 1 workflow with the jobs data
// So, we don't want to merge these two queries

iii. Queries must reuse/share fragments. So that we can define what is a Job and use it in a Tree view query, or Graph view query, etc (this is partially done already).

Pull requests welcome!

@kinow kinow added this to the 0.3 milestone Apr 2, 2020
@kinow kinow self-assigned this Apr 2, 2020
@kinow
Copy link
Member Author

kinow commented Apr 3, 2020

Would it work with Cylc UI (i.e. Vue.js, ApolloClient, Babel, Webpack, etc, etc?)

First PoC, to confirm it works with Cylc UI.

https://github.com/kinow/cylc-ui/compare/master..graphql-tag-1

GIFrecord_2020-04-03_141041

(disabled GScan as otherwise it would try to merge string+gql.)

So instead of parsing String to JSON, and then merging objects in JS, it would instead assume views want to subscribe a Query or Subscription, e.g. for the Tree View specific view (not lumino)

          subscription TreeView ($ids: [ID]) {
            workflows (ids: $ids) {
              ...workflowData
              ...treeData
            }
          }

That could actually be the root query of the WorkflowService.

Then the view must add the fragments. The equivalent query in GraphiQL would be

subscription TreeView ($ids: [ID]) {
  workflows (ids: $	ids) {
    ...workflowData
    ...treeData
  }
}

fragment workflowData on Workflow {
  id
  name
  status
  owner
  host
  port
}

fragment treeData on Workflow {
  cyclePoints: familyProxies(ids: ["root"]) {
    cyclePoint
  }
  taskProxies(sort: { keys: ["cyclePoint"] }) {
    id
    name
    state
    cyclePoint
    latestMessage
    firstParent {
      id
      name
      cyclePoint
      state
    }
    task {
      meanElapsedTime
      name
    }
    jobs(sort: { keys: ["submit_num"], reverse:true }) {
      id
      batchSysName
      batchSysJobId
      host
      startedTime
      submittedTime
      finishedTime
      state
      submitNum
    }
  }
  familyProxies (exids: ["root"], sort: { keys: ["firstParent"]}) {
    id
    name
    state
    cyclePoint
    firstParent {
      id
      name
      cyclePoint
      state
    }
  }
}

With variables:

{
  "ids": ["kinow|five"]
}

@kinow
Copy link
Member Author

kinow commented Apr 3, 2020

Would we be able to merge the objects?

See related issues/comments, which show that we are not the only ones trying to achieve this.

Was going to start working on a Codepen example, but turns out graphql-tag is not available via CDN's. Will use Cylc UI for a small simple example.

To be continued... (had a team meeting just before end of working day, so will continue next week 👍 )

Each view would use 1 query (so far, every view we have use the same query/subscription type of Workflow). For example.

subscription {
  workflows {
    id
    name
    // fragments expanded here
  }
}

//fragments go here

Then besides the query of the view, each view (or component) would tell the WorkflowService what fragments it wants to use.

We would only need to replace where it says that fragments go, or are expanded. The final query would look like:

subscription {
  workflows {
    id
    name
    ...View1Data
    ...View2Data
  }
}

fragment View1Data on Workflow {
  status
  taskProxies {
    jobs (sort: { keys: ["submit_num"], reverse:true }) {
      id
    }
  }
}

fragment View2Data on Workflow {
  port
  taskProxies {
    jobs (sort: { keys: ["submit_num"], reverse:true }) {
      id
      cyclePoint
    }
  }
}

Why is that a good idea?

  1. If you combine two fragments that have incompatible arguments, GraphQL fails. This error would be detected during development time. At the moment the query is merged and the right term of the operation (i.e. merge( left-query, right-query)) is the value used, replacing anything in the left term.

IOW, if you have:

jobs (sort: { keys: ["submit_num"], reverse:true }) {
  id
}

And you have another fragment with

jobs {
  id
}

The query fails to run, so unit and/or functional tests will fail. Which is better than having possible undesired behaviour in runtime (if not caught by any tests).

{
  "id": 9,
  "type": "data",
  "payload": {
    "errors": [
      {
        "message": "Fields \"taskProxies\" conflict because subfields \"jobs\" conflict because they have differing arguments. Use different aliases on the fields to fetch both if this was intentional.",
        "locations": [
          {
            "line": 12,
            "column": 3
          },
          {
            "line": 13,
            "column": 5
          },
          {
            "line": 21,
            "column": 3
          },
          {
            "line": 22,
            "column": 5
          }
        ]
      },
      {
        "message": "Field \"cyclePoint\" of type \"String!\" must not have a sub selection.",
        "locations": [
          {
            "line": 24,
            "column": 18
          }
        ]
      }
    ]
  }
}
  1. Easier to guess what's the final query. As we can simply copy the query and the fragments, then we only have to write ...FragmentName and paste each fragment after the query, and see the result in GraphiQL.

  2. Variables are supported. At the moment we don't support variables in the merged query. Using graphql-tag, we can pass the JSON object to the ApolloClient, or even use the beta fragment variables.

The major downside for this approach is that it requires quite a lot of modifications in the WorkflowService and views/components using it.

So probably an issue for 0.3 or later.

@kinow kinow mentioned this issue May 13, 2020
6 tasks
kinow added a commit to kinow/cylc-ui that referenced this issue May 22, 2020
kinow added a commit to kinow/cylc-ui that referenced this issue May 22, 2020
kinow added a commit to kinow/cylc-ui that referenced this issue May 28, 2020
@kinow kinow removed their assignment Jul 5, 2020
@oliver-sanders oliver-sanders modified the milestones: 0.3, 0.4 Mar 4, 2021
@oliver-sanders oliver-sanders modified the milestones: 0.4.0, 0.5.0 Apr 16, 2021
@kinow kinow pinned this issue Jun 3, 2021
@kinow kinow self-assigned this Jun 3, 2021
@kinow
Copy link
Member Author

kinow commented Jun 9, 2021

Yesterday created some code to merge queries programmatically. It stops shy of working, just needs to merge the selection nodes.

A GraphQL query has an operation definition, that contains a selection set. The selection set includes fields and fragments, recursively. This is the tricky part... I found some libraries that could be used to simplify it. But I realized there could be a simpler approach. But the work is saved in a branch.

https://github.com/kinow/cylc-ui/tree/graphql-tag-2

@kinow
Copy link
Member Author

kinow commented Jun 9, 2021

Today I started the different approach, using a higher order component. What's interesting about this approach, is that it requires changes that are in other issues/pull requests. Like better organize the mixins, which was done in #672 (I cherry-picked commits from that branch), and also have a unique way to handle deltas, which would also fix #684 .

The HOC function is almost ready for a test I think, but still need to combine fragments. The idea is, instead of merging objects, which can be hard, we simply add the fragments to a set. And then expand the base query. This base query has the following form:

subscription ($workflowId: ID) {
  deltas (workflows: [$workflowId], stripNull: true) {
    id
    shutdown
    added {
      ...AddedData
    }
    updated {
      ...UpdatedData
    }
    pruned {
      ...PrunedData
    }
  }
}

fragment AddedData on Added {
  workflow {
    ...WorkflowData
  }
}

fragment UpdatedData on Updated {
  workflow {
    ...WorkflowData
  }
}

fragment PrunedData on Pruned {
  workflow
}

fragment WorkflowData on Workflow {
  id
  name
  status
  owner
  host
  port
}

This query retrieves workflows, in a deltas-subscription, for a single workflow (filtered by a variable). What I am aiming for, in this branch, is to have a view like views/Tree.vue being created with the HOC function, and incrementing the HOC component with something like:

# views/Tree.vue
{
  // rest of component here or inherited from HOC function, combined/merged with this view
  data () {
    fragments: {
      taskProxies: gql`fragment TaskProxiesAddedData on Added {
        taskProxies (ghosts: true) {
          ...TaskProxyData
        }
      }`,
      taskProxyData: TaskProxyData // we can import it from graphql/queries.js
    }
  }
}

And then programmatically the component, upon creation, will know that it needs to combine all the fragments provided, creating the final GraphQL query to be used in the subscription.

Work being pushed to https://github.com/kinow/cylc-ui/tree/graphql-tag-3

@hjoliver
Copy link
Member

@kinow
Copy link
Member Author

kinow commented Jun 10, 2021

@kinow have you seen this? https://github.com/domasx2/graphql-combine-query

Yup, but thanks for posting it here! It was one of the libs I found when searching for someone doing the same thing we are doing. I will explain tonight the current approach. It's not merging queries, instead it's combining fragments. But there's one downside that I want to check with others.

If we really have to merge it, then I will either convert queries to JSON and merge (what we do today), implement the merging of fragments/selection sets (fields of a query), or use that library 👍

@kinow
Copy link
Member Author

kinow commented Jun 10, 2021

(that library appeared in a few places actually, SO answers and GH replies; for more: #439 (comment))

@kinow
Copy link
Member Author

kinow commented Aug 9, 2021

Testing domasx2/graphql-combine-query. Code looks simple, updated our code to use their API, and it compiled OK. There's a runtime error now, just need to understand why it's happening.

image

@kinow
Copy link
Member Author

kinow commented Aug 9, 2021

domasx2/graphql-combine-query can be used to merge multiple documents with different fields. Our use case is different. We have views that queries/documents that may share fields. i.e. in the Workflow view, it may include a Tree and a Table views, both using the deltas field (which fails in graphql-combine-query).

@kinow
Copy link
Member Author

kinow commented Aug 9, 2021

https://github.com/svt/graphql-defragmentizer looks useful, but still not quite what we want. From what I understood, it would require us to create JS objects that are then combined. It's an extra layer on top of graphql-tag that produces a valid GraphQL query. Didn't test it because it doesn't appear to be actively maintained, and because the Insights on GitHub didn't show any dependants (i.e. no users).

@kinow kinow mentioned this issue Aug 9, 2021
6 tasks
@kinow kinow unpinned this issue Sep 28, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants