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

mutations not loading correctly #998

Closed
oliver-sanders opened this issue Apr 29, 2022 · 10 comments · Fixed by #999
Closed

mutations not loading correctly #998

oliver-sanders opened this issue Apr 29, 2022 · 10 comments · Fixed by #999
Assignees
Labels
bug Something isn't working
Milestone

Comments

@oliver-sanders
Copy link
Member

The same issue fixed in #979 seems to still be happening, I'm not sure how.

The mutations are loaded via a GraphQL request to the server. #979 made this more async friendly to hand the case where a mutation is issued before the list of mutations has been received.

The introspection query (which loads the mutations) has been issued:

Screenshot from 2022-04-29 10-47-57

The response has been received:

Screenshot from 2022-04-29 10-48-15

Yet the mutations have not been set for some reason?

Screenshot from 2022-04-29 10-48-37

The tests are passing, it all works with mocked data - https://github.com/cylc/cylc-ui/runs/6225942695?check_suite_focus=true - but seems to fail in the real world.

If I leave the page alone for a few minutes the issue eventually resolves itself:

XHRPOSThttp://localhost:8888/cylc/graphql [HTTP/1.1 200 OK 2826ms]

I'm not familiar with the JS async interface, perhaps we need to give it a poke somehow? On the surface it looks to me like the .then isn't happening until a lot later than anticipated.

Pull requests welcome!
This is an Open Source project - please consider contributing a bug fix
yourself (please read CONTRIBUTING.md before starting any work though).

@oliver-sanders oliver-sanders added the bug Something isn't working label Apr 29, 2022
@oliver-sanders oliver-sanders added this to the 1.2.0 milestone Apr 29, 2022
@datamel
Copy link
Contributor

datamel commented Apr 29, 2022

I have reproduced this error.

@oliver-sanders
Copy link
Member Author

(Ping @MetRonnie who might have some ideas)

I think this might be related to the different pathways taken for a mutation issued from the toolbar play/pause/stop icons (which go straight to issuing the mutation) to the mutation menu which has better fail-safe features.

@oliver-sanders
Copy link
Member Author

Looks like for toolbar usage the mutation is coming through as a Promise, but when I try to resolve it I get undefined:

getMutationArgsFromTokens1([object Promise], [object Object])
getMutationArgsFromTokens2(undefined, [object Object])
--- a/src/utils/aotf.js
+++ b/src/utils/aotf.js
@@ -611,30 +611,34 @@ export function getMutationArgsFromTokens (mutation, tokens) {
   const argspec = {}
   let value = null
   let alternate = null
-  for (const arg of mutation.args) {
-    alternate = alternateFields[arg._cylcType]
-    for (let token in tokens) {
-      if (arg._cylcObject && [token, alternate].indexOf(arg._cylcObject) >= 0) {
-        if (arg._cylcObject === alternate) {
-          token = alternate
-        }
-        if (arg._cylcType in compoundFields) {
-          value = compoundFields[arg._cylcType](tokens)
-        } else {
-          value = tokens[token]
-        }
-        if (arg._multiple) {
-          value = [value]
+  console.log(`getMutationArgsFromTokens1(${mutation}, ${tokens})`)
+  mutation.then((mutation) => {
+    console.log(`getMutationArgsFromTokens2(${mutation}, ${tokens})`)
+    for (const arg of mutation.args) {
+      alternate = alternateFields[arg._cylcType]
+      for (let token in tokens) {
+        if (arg._cylcObject && [token, alternate].indexOf(arg._cylcObject) >= 0) {
+          if (arg._cylcObject === alternate) {
+            token = alternate
+          }
+          if (arg._cylcType in compoundFields) {
+            value = compoundFields[arg._cylcType](tokens)
+          } else {
+            value = tokens[token]
+          }
+          if (arg._multiple) {
+            value = [value]
+          }
+          argspec[arg.name] = value
+          break
         }
-        argspec[arg.name] = value
-        break
+      }
+      if (!argspec[arg.name]) {
+        argspec[arg.name] = arg._default
       }
     }
-    if (!argspec[arg.name]) {
-      argspec[arg.name] = arg._default
-    }
-  }
-  return argspec
+    return argspec
+  })
 }

@MetRonnie
Copy link
Member

How to reproduce? Is this bug only happening when clicking the toolbar play/pause/stop icons?

@oliver-sanders
Copy link
Member Author

Is this bug only happening when clicking the toolbar play/pause/stop icons?

I think so.

@oliver-sanders
Copy link
Member Author

Might just be missing an await/then statement in the WorkflowService.mutate pathway.

@oliver-sanders
Copy link
Member Author

Something along the lines of this (only implemented by someone who actually understands async JS):

diff --git a/src/components/cylc/workflow/Toolbar.vue b/src/components/cylc/workflow/Toolbar.vue
index 3c1590bd..ede8fc38 100644
--- a/src/components/cylc/workflow/Toolbar.vue
+++ b/src/components/cylc/workflow/Toolbar.vue
@@ -260,13 +260,14 @@ export default {
   },
   methods: {
     onClickPlay () {
-      const ret = this.$workflowService.mutate(
+      this.$workflowService.mutate(
         'play',
         this.currentWorkflow.id
-      )
-      if (ret[0] === mutationStatus.SUCCEEDED) {
-        this.expecting.play = !this.isRunning
-      }
+      ).then(ret => {
+        if (ret[0] === mutationStatus.SUCCEEDED) {
+          this.expecting.play = !this.isRunning
+        }
+      })
     },
     onClickReleaseHold () {
       const ret = this.$workflowService.mutate(
diff --git a/src/services/workflow.service.js b/src/services/workflow.service.js
index eecf5de9..8ca821e4 100644
--- a/src/services/workflow.service.js
+++ b/src/services/workflow.service.js
@@ -87,8 +87,9 @@ class WorkflowService {
    * @param {String} id
    * @returns {Promise}
    */
-  mutate (mutationName, id) {
-    const mutation = this.getMutation(mutationName)
+  async mutate (mutationName, id) {
+    const mutation = await this.getMutation(mutationName)
+    console.log(`mutation=${mutation}`)
     return mutate(
       mutation,
       getMutationArgsFromTokens(
@@ -122,8 +123,9 @@ class WorkflowService {
    *
    * @param {String} mutationName
    */
-  getMutation (mutationName) {
-    return this.mutations.find(mutation => mutation.name === mutationName)
+  async getMutation (mutationName) {
+    const [mutations, types] = await this.mutationsAndTypes
+    return mutations.find(mutation => mutation.name === mutationName)
   }
 
   // --- GraphQL query subscriptions

@MetRonnie
Copy link
Member

MetRonnie commented Apr 29, 2022

I get

vue.esm.js:1906 TypeError: Cannot read properties of undefined (reading 'find')

on Chrome. Might be a V8 vs SpiderMonkey thing, as the error fixed by #979 also had a different message on Chrome vs Firefox.

Edit: found the source, think I missed a necessary change in #979

@oliver-sanders
Copy link
Member Author

oliver-sanders commented Apr 29, 2022

I think this has done it - #999

diff --git a/src/components/cylc/workflow/Toolbar.vue b/src/components/cylc/workflow/Toolbar.vue
index 3c1590bd..ede8fc38 100644
--- a/src/components/cylc/workflow/Toolbar.vue
+++ b/src/components/cylc/workflow/Toolbar.vue
@@ -260,13 +260,14 @@ export default {
   },
   methods: {
     onClickPlay () {
-      const ret = this.$workflowService.mutate(
+      this.$workflowService.mutate(
         'play',
         this.currentWorkflow.id
-      )
-      if (ret[0] === mutationStatus.SUCCEEDED) {
-        this.expecting.play = !this.isRunning
-      }
+      ).then(ret => {
+        if (ret[0] === mutationStatus.SUCCEEDED) {
+          this.expecting.play = !this.isRunning
+        }
+      })
     },
     onClickReleaseHold () {
       const ret = this.$workflowService.mutate(
diff --git a/src/services/workflow.service.js b/src/services/workflow.service.js
index eecf5de9..0e1de1de 100644
--- a/src/services/workflow.service.js
+++ b/src/services/workflow.service.js
@@ -87,8 +87,9 @@ class WorkflowService {
    * @param {String} id
    * @returns {Promise}
    */
-  mutate (mutationName, id) {
-    const mutation = this.getMutation(mutationName)
+  async mutate (mutationName, id) {
+    const mutation = await this.getMutation(mutationName)
+    console.log(`mutation=${mutation}`)
     return mutate(
       mutation,
       getMutationArgsFromTokens(
@@ -122,8 +123,9 @@ class WorkflowService {
    *
    * @param {String} mutationName
    */
-  getMutation (mutationName) {
-    return this.mutations.find(mutation => mutation.name === mutationName)
+  async getMutation (mutationName) {
+    const { mutations, types } = await this.mutationsAndTypes
+    return mutations.find(mutation => mutation.name === mutationName)
   }
 
   // --- GraphQL query subscriptions

@MetRonnie
Copy link
Member

Looks reasonable

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants