Serialize sessionTraceData to Bindings by QilongTang · Pull Request #7941 · DynamoDS/Dynamo

QilongTang · 2017-06-08T19:16:09Z

Purpose

QNTM-701
In Dynamo Core, Graph.JSON should have "Bindings" instead of "sessionTraceData"

This PR:

Deletes the existing sessionTraceData xml serialization logic
Append bindings serialization logic to workspace serialization

Declarations

Check these if you believe they are true

The code base is in a better state after this PR
Is documented according to the standards
The level of testing this PR includes is appropriate
User facing strings, if any, are extracted into *.resx files
All tests pass using the self-service CI.
Snapshot of UI changes, if any.
Changes to the API follow Semantic Versioning, and are documented in the API Changes document.

Reviewers

@mjkkirschner @ramramps @gregmarr

FYIs

@ikeough

…ize-sessionTraceData # Conflicts: # src/AssemblySharedInfoGenerator/AssemblySharedInfo.cs

mjkkirschner · 2017-06-08T19:17:57Z

-        internal static readonly string SessionTraceDataXmlTag = "SessionTraceData";
-        internal static readonly string NodeTraceDataXmlTag = "NodeTraceData";
-        internal static readonly string CallsiteTraceDataXmlTag = "CallsiteTraceData";
+        internal static readonly string SessionTraceDataTag = "Bindings";


will changing this string break xml deserialization of session data?

Good point. I think unfortunately, yes. LoadTraceDataFromXmlDocument will still be used for legacy dyn opening as part of DynamoCore 2.0 and it will be checking the old xml tag from the old dyns. I will revert the related changes. For the new json block bindings, I can hardcode the json block name just like the other ones. But should we also consider making a config file to store all these json property names? @ikeough

why don't you just add an additional const and then use the json one in the json serialization and the xml one in xml deserialization...
I'm also curious did you try adding a serializer specifically for the callSiteTrace data class - or attaching a converter that class directly to json.

My preference would be to avoid adding more and more logic to this workspace converter.

…ize-sessionTraceData

QilongTang · 2017-06-12T20:17:24Z

With the changes in the PR, the serialized data currently looks like this.

"Bindings": [
    {
      "NodeTraceData NodeId": "a36a5bdc-8c94-41e0-9247-e5cc9a9a3de7",
      "CallsiteTraceData": [
        {
          "CallSiteID ByCurve_InClassDecl-1_InFunctionScope-1_Instance0_a36a5bdc-8c94-41e0-9247-e5cc9a9a3de7": "PFNPQVAtRU5WOkVudmVsb3BlIHhtbG5zOnhzaT0iaHR0cDovL3d3dy53My5vcmcvMjAwMS9YTUxTY2hlbWEtaW5zdGFuY2UiIHhtbG5zOnhzZD0iaHR0cDovL3d3dy53My5vcmcvMjAwMS9YTUxTY2hlbWEiIHhtbG5zOlNPQVAtRU5DPSJodHRwOi8vc2NoZW1hcy54bWxzb2FwLm9yZy9zb2FwL2VuY29kaW5nLyIgeG1sbnM6U09BUC1FTlY9Imh0dHA6Ly9zY2hlbWFzLnhtbHNvYXAub3JnL3NvYXAvZW52ZWxvcGUvIiB4bWxuczpjbHI9Imh0dHA6Ly9zY2hlbWFzLm1pY3Jvc29mdC5jb20vc29hcC9lbmNvZGluZy9jbHIvMS4wIiBTT0FQLUVOVjplbmNvZGluZ1N0eWxlPSJodHRwOi8vc2NoZW1hcy54bWxzb2FwLm9yZy9zb2FwL2VuY29kaW5nLyI+DQo8U09BUC1FTlY6Qm9keT4NCjxhMTpDYWxsU2l0ZV94MDAyQl9UcmFjZVNlcmlhbGlzZXJIZWxwZXIgaWQ9InJlZi0xIiB4bWxuczphMT0iaHR0cDovL3NjaGVtYXMubWljcm9zb2Z0LmNvbS9jbHIvbnNhc3NlbS9Qcm90b0NvcmUvUHJvdG9Db3JlJTJDJTIwVmVyc2lvbiUzRDIuMC4wLjUyODUlMkMlMjBDdWx0dXJlJTNEbmV1dHJhbCUyQyUyMFB1YmxpY0tleVRva2VuJTNEbnVsbCI+DQo8TnVtYmVyT2ZFbGVtZW50cz4xPC9OdW1iZXJPZkVsZW1lbnRzPg0KPEJhc2UtMF9IYXNEYXRhPnRydWU8L0Jhc2UtMF9IYXNEYXRhPg0KPEJhc2UtMF9EYXRhIGlkPSJyZWYtMyI+UEZOUFFWQXRSVTVXT2tWdWRtVnNiM0JsSUhodGJHNXpPbmh6YVQwaWFIUjBjRG92TDNkM2R5NTNNeTV2Y21jdk1qQXdNUzlZVFV4VFkyaGxiV0V0YVc1emRHRnVZMlVpSUhodGJHNXpPbmh6WkQwaWFIUjBjRG92TDNkM2R5NTNNeTV2Y21jdk1qQXdNUzlZVFV4VFkyaGxiV0VpSUhodGJHNXpPbE5QUVZBdFJVNURQU0pvZEhSd09pOHZjMk5vWlcxaGN5NTRiV3h6YjJGd0xtOXlaeTl6YjJGd0wyVnVZMjlrYVc1bkx5SWdlRzFzYm5NNlUwOUJVQzFGVGxZOUltaDBkSEE2THk5elkyaGxiV0Z6TG5odGJITnZZWEF1YjNKbkwzTnZZWEF2Wlc1MlpXeHZjR1V2SWlCNGJXeHVjenBqYkhJOUltaDBkSEE2THk5elkyaGxiV0Z6TG0xcFkzSnZjMjltZEM1amIyMHZjMjloY0M5bGJtTnZaR2x1Wnk5amJISXZNUzR3SWlCVFQwRlFMVVZPVmpwbGJtTnZaR2x1WjFOMGVXeGxQU0pvZEhSd09pOHZjMk5vWlcxaGN5NTRiV3h6YjJGd0xtOXlaeTl6YjJGd0wyVnVZMjlrYVc1bkx5SStEUW84VTA5QlVDMUZUbFk2UW05a2VUNE5DanhoTVRwVFpYSnBZV3hwZW1GaWJHVkpaQ0JwWkQwaWNtVm1MVEVpSUhodGJHNXpPbUV4UFNKb2RIUndPaTh2YzJOb1pXMWhjeTV0YVdOeWIzTnZablF1WTI5dEwyTnNjaTl1YzJGemMyVnRMMUpsZG1sMFUyVnlkbWxqWlhNdVVHVnljMmx6ZEdWdVkyVXZVbVYyYVhSVFpYSjJhV05sY3lVeVF5VXlNRlpsY25OcGIyNGxNMFF5TGpBdU1DNDFNalUxSlRKREpUSXdRM1ZzZEhWeVpTVXpSRzVsZFhSeVlXd2xNa01sTWpCUWRXSnNhV05MWlhsVWIydGxiaVV6Ukc1MWJHd2lQZzBLUEhOMGNtbHVaMGxFSUdsa1BTSnlaV1l0TXlJK09HTmtNREJsTlRRdFpEUXpNeTAwWlRWaExUazJPV1V0WmpKa1pqQTJNbU0yWVdKaExUQXdNVEE1TkRneVBDOXpkSEpwYm1kSlJENE5DanhwYm5SSlJENHhNRGcyTlRrMFBDOXBiblJKUkQ0TkNqd3ZZVEU2VTJWeWFXRnNhWHBoWW14bFNXUStEUW84TDFOUFFWQXRSVTVXT2tKdlpIaytEUW84TDFOUFFWQXRSVTVXT2tWdWRtVnNiM0JsUGcwSzwvQmFzZS0wX0RhdGE+DQo8QmFzZS0wX0hhc05lc3RlZERhdGE+ZmFsc2U8L0Jhc2UtMF9IYXNOZXN0ZWREYXRhPg0KPC9hMTpDYWxsU2l0ZV94MDAyQl9UcmFjZVNlcmlhbGlzZXJIZWxwZXI+DQo8L1NPQVAtRU5WOkJvZHk+DQo8L1NPQVAtRU5WOkVudmVsb3BlPg0K"
        }
      ]
    }
  ]

I suspect we can simplify the format even further like the following but would like to gather some thoughts around this @ramramps @mjkkirschner @sharadkjaiswal :

"Bindings": [
    {
      "NodeId": "a36a5bdc-8c94-41e0-9247-e5cc9a9a3de7",
      "CallsiteTraceData": [
        {
          "ByCurve_InClassDecl-1_InFunctionScope-1_Instance0_a36a5bdc-8c94-41e0-9247-e5cc9a9a3de7": "PFNPQVAtRU5WOkVudmVsb3BlIHhtbG5zOnhzaT0iaHR0cDovL3d3dy53My5vcmcvMjAwMS9YTUxTY2hlbWEtaW5zdGFuY2UiIHhtbG5zOnhzZD0iaHR0cDovL3d3dy53My5vcmcvMjAwMS9YTUxTY2hlbWEiIHhtbG5zOlNPQVAtRU5DPSJodHRwOi8vc2NoZW1hcy54bWxzb2FwLm9yZy9zb2FwL2VuY29kaW5nLyIgeG1sbnM6U09BUC1FTlY9Imh0dHA6Ly9zY2hlbWFzLnhtbHNvYXAub3JnL3NvYXAvZW52ZWxvcGUvIiB4bWxuczpjbHI9Imh0dHA6Ly9zY2hlbWFzLm1pY3Jvc29mdC5jb20vc29hcC9lbmNvZGluZy9jbHIvMS4wIiBTT0FQLUVOVjplbmNvZGluZ1N0eWxlPSJodHRwOi8vc2NoZW1hcy54bWxzb2FwLm9yZy9zb2FwL2VuY29kaW5nLyI+DQo8U09BUC1FTlY6Qm9keT4NCjxhMTpDYWxsU2l0ZV94MDAyQl9UcmFjZVNlcmlhbGlzZXJIZWxwZXIgaWQ9InJlZi0xIiB4bWxuczphMT0iaHR0cDovL3NjaGVtYXMubWljcm9zb2Z0LmNvbS9jbHIvbnNhc3NlbS9Qcm90b0NvcmUvUHJvdG9Db3JlJTJDJTIwVmVyc2lvbiUzRDIuMC4wLjUyODUlMkMlMjBDdWx0dXJlJTNEbmV1dHJhbCUyQyUyMFB1YmxpY0tleVRva2VuJTNEbnVsbCI+DQo8TnVtYmVyT2ZFbGVtZW50cz4xPC9OdW1iZXJPZkVsZW1lbnRzPg0KPEJhc2UtMF9IYXNEYXRhPnRydWU8L0Jhc2UtMF9IYXNEYXRhPg0KPEJhc2UtMF9EYXRhIGlkPSJyZWYtMyI+UEZOUFFWQXRSVTVXT2tWdWRtVnNiM0JsSUhodGJHNXpPbmh6YVQwaWFIUjBjRG92TDNkM2R5NTNNeTV2Y21jdk1qQXdNUzlZVFV4VFkyaGxiV0V0YVc1emRHRnVZMlVpSUhodGJHNXpPbmh6WkQwaWFIUjBjRG92TDNkM2R5NTNNeTV2Y21jdk1qQXdNUzlZVFV4VFkyaGxiV0VpSUhodGJHNXpPbE5QUVZBdFJVNURQU0pvZEhSd09pOHZjMk5vWlcxaGN5NTRiV3h6YjJGd0xtOXlaeTl6YjJGd0wyVnVZMjlrYVc1bkx5SWdlRzFzYm5NNlUwOUJVQzFGVGxZOUltaDBkSEE2THk5elkyaGxiV0Z6TG5odGJITnZZWEF1YjNKbkwzTnZZWEF2Wlc1MlpXeHZjR1V2SWlCNGJXeHVjenBqYkhJOUltaDBkSEE2THk5elkyaGxiV0Z6TG0xcFkzSnZjMjltZEM1amIyMHZjMjloY0M5bGJtTnZaR2x1Wnk5amJISXZNUzR3SWlCVFQwRlFMVVZPVmpwbGJtTnZaR2x1WjFOMGVXeGxQU0pvZEhSd09pOHZjMk5vWlcxaGN5NTRiV3h6YjJGd0xtOXlaeTl6YjJGd0wyVnVZMjlrYVc1bkx5SStEUW84VTA5QlVDMUZUbFk2UW05a2VUNE5DanhoTVRwVFpYSnBZV3hwZW1GaWJHVkpaQ0JwWkQwaWNtVm1MVEVpSUhodGJHNXpPbUV4UFNKb2RIUndPaTh2YzJOb1pXMWhjeTV0YVdOeWIzTnZablF1WTI5dEwyTnNjaTl1YzJGemMyVnRMMUpsZG1sMFUyVnlkbWxqWlhNdVVHVnljMmx6ZEdWdVkyVXZVbVYyYVhSVFpYSjJhV05sY3lVeVF5VXlNRlpsY25OcGIyNGxNMFF5TGpBdU1DNDFNalUxSlRKREpUSXdRM1ZzZEhWeVpTVXpSRzVsZFhSeVlXd2xNa01sTWpCUWRXSnNhV05MWlhsVWIydGxiaVV6Ukc1MWJHd2lQZzBLUEhOMGNtbHVaMGxFSUdsa1BTSnlaV1l0TXlJK09HTmtNREJsTlRRdFpEUXpNeTAwWlRWaExUazJPV1V0WmpKa1pqQTJNbU0yWVdKaExUQXdNVEE1TkRneVBDOXpkSEpwYm1kSlJENE5DanhwYm5SSlJENHhNRGcyTlRrMFBDOXBiblJKUkQ0TkNqd3ZZVEU2VTJWeWFXRnNhWHBoWW14bFNXUStEUW84TDFOUFFWQXRSVTVXT2tKdlpIaytEUW84TDFOUFFWQXRSVTVXT2tWdWRtVnNiM0JsUGcwSzwvQmFzZS0wX0RhdGE+DQo8QmFzZS0wX0hhc05lc3RlZERhdGE+ZmFsc2U8L0Jhc2UtMF9IYXNOZXN0ZWREYXRhPg0KPC9hMTpDYWxsU2l0ZV94MDAyQl9UcmFjZVNlcmlhbGlzZXJIZWxwZXI+DQo8L1NPQVAtRU5WOkJvZHk+DQo8L1NPQVAtRU5WOkVudmVsb3BlPg0K"
        }
      ]
    }
  ]

mjkkirschner · 2017-06-12T20:29:55Z

@QilongTang both of those look a little funny.
Have you tried replicating a node and seeing what the output looks like?

If you remove these property names, will it be difficult these deserialize this objects?

As ugly as it is, it seems this should be a series of nested objects.

gregmarr · 2017-06-12T20:32:13Z

This is very similar to what I was just proposing to Ian on Slack.

Quote:

Bindings:
  type: array
  items:
    type: object
    required:
    - node_id
    - binding
    properties:
      node_id:
        format: string
      binding:
        format: string

or something like this:

      Bindings:
        additionalProperties:
          type: array
          items:
            type: string

Looks like this latter one does not seem to show up properly in swagger-ui until 3.0 though.
The latter is a plain bucket of random name-string pairs.

Bindings: [ "foo": "bar", "baz": "blargh"]

The former is more structured:

Bindings: [ { "node_id": "foo", "binding": "bar" }, { "node_id": "baz", "binding": "blargh" } ]

and allows for future expansion if we need to add a second value for some reason.

We could theoretically combine the two, and have "binding" be the place for the additionalProperties, so it's one node id, and then a map of key-value pairs, so you could have a "D4Revit" binding, and a "D4Rhino" binding.

End Quote

Looks like we already need the last one.

    Bindings:
      type: array
      items:
        $ref: "#/definitions/Binding"

Binding:
  type: object
  required:
  - NodeId
  properties:
    NodeId:
      format: string
  additionalProperties:
    type: array
    items:
      type: string

gregmarr · 2017-06-12T20:36:45Z

I used "string" as the value type in additionalProperties because #7747 says that the values are base64 encoded strings. Do we need to change that?
Actually, as written above it's an array of strings, it should probably actually just be this based on #7747

  additionalProperties:
   type: string

mjkkirschner · 2017-06-12T20:40:42Z

@gregmarr I might be mistaken, but per node ID I think there can be multiple callsite bindings (atleast in DynamoRevit) @QilongTang can you confirm or deny that?, so I guess we're missing an array of bindings per node, unless the binding object can be whatever type is required. - more like the additional properties one.

QilongTang · 2017-06-12T20:43:01Z

@mjkkirschner Yes, per node Id there can be multiple callSite bindings. That's why I put CallsiteTraceData as an array of object in the first pass. @gregmarr string seems fine

QilongTang · 2017-06-13T04:17:17Z

@mjkkirschner @gregmarr Updated the format to remove the property names and add binding, it looks like this right now as a combination of the two in your quote. What do you think? @sharadkjaiswal @Randy-Ma Is the id for callSiteRawData unique so I can store it this way?

"Bindings": [
    {
      "NodeId": "a36a5bdc-8c94-41e0-9247-e5cc9a9a3de7",
      "Binding": [
        {
          "callSiteTraceDataId1": "callSiteTraceData1",
          "callSiteTraceDataId2": "callSiteTraceData2",
        },
       { Rihno binding },
       { Other binding}
      ]
    },
    {
      "NodeId": "1825d7c7-a85c-4065-bafb-2b97c38a9feb",
      "Binding": [
        {
          "callSiteTraceDataId1": "callSiteTraceData1",
          "callSiteTraceDataId2": "callSiteTraceData2",
        },
       { Rihno binding },
       { Other binding}
      ]
    }
  ]

gregmarr · 2017-06-13T12:28:06Z

That looks good to me.

QilongTang · 2017-06-13T13:26:45Z

@gregmarr Do you imagine we serialize client Name in each Binding block as well? I assume in this way the deserialization would be challenging that client need to figure out which block of binding belongs to itself

gregmarr · 2017-06-13T13:33:49Z

I just noticed that there is different nesting there than I thought before. This is what I would expect with a schema of

Binding:
  type: object
  required:
  - NodeId
  properties:
    NodeId:
      format: string
  additionalProperties:
    type: array
    items:
      type: string

"Bindings": [
    {
      "NodeId": "a36a5bdc-8c94-41e0-9247-e5cc9a9a3de7",
      "Binding":
        {
          "callSiteTraceDataId1": ["callSiteTraceData1"],
          "callSiteTraceDataId2": ["callSiteTraceData2"],
          "Rhino binding": ["foo", "bar"],
          "Other binding": []
        }
    },
    {
...
    }
  ]

If we don't need the array there, and it's one key to one value, then it's just

Binding:
  type: object
  required:
  - NodeId
  properties:
    NodeId:
      format: string
  additionalProperties:
    type: string

and

"Bindings": [
    {
      "NodeId": "a36a5bdc-8c94-41e0-9247-e5cc9a9a3de7",
      "Binding":
        {
          "callSiteTraceDataId1": "callSiteTraceData1",
          "callSiteTraceDataId2": "callSiteTraceData2",
          "Rhino binding": "foo",
          "Other binding": "bar"
        }
    },
    {
...
    }
  ]

QilongTang · 2017-06-13T14:34:32Z

@gregmarr Removed the extra array layer, I think it is not necessary right now, so it aligns with your expectation:

"Bindings": [
    {
      "NodeId": "a36a5bdc-8c94-41e0-9247-e5cc9a9a3de7",
      "Binding":
        {
          "callSiteTraceDataId1": "callSiteTraceData1",
          "callSiteTraceDataId2": "callSiteTraceData2",
          "Rhino binding": "foo",
          "Other binding": "bar"
        }
    },
    {
...
    }
  ]

gregmarr · 2017-06-13T14:35:24Z

Perfect, thanks.

QilongTang · 2017-06-13T14:54:46Z

A follow up task https://jira.autodesk.com/browse/QNTM-870 created as part of enabling opening such JSON format in D4R. Merging.

QilongTang added 5 commits June 7, 2017 14:14

First commit

d071056

CleanUp

9b1f2d5

Remove SaveTraceDataToJson()

f6c624f

Merge remote-tracking branch 'refs/remotes/origin/master' into Serial…

004599c

…ize-sessionTraceData # Conflicts: # src/AssemblySharedInfoGenerator/AssemblySharedInfo.cs

CleanUp

d3d9725

QilongTang added the WIP label Jun 8, 2017

mjkkirschner reviewed Jun 8, 2017

View reviewed changes

QilongTang added 4 commits June 8, 2017 15:32

Revert change again

f149361

CleanUp

730e581

Revert change to assemblySharedInfo :(

cc798ba

remove unused reference

8e9589f

mjkkirschner added the JSON label Jun 9, 2017

QilongTang added 6 commits June 9, 2017 09:56

Update tags in Config

4e90f03

Disable Serialization for DropDownItems

a7b6af4

Clean Up

30a0171

CleanUp

f8213ae

Merge remote-tracking branch 'refs/remotes/origin/master' into Serial…

98a4616

…ize-sessionTraceData

CleanUp

08d8e74

QilongTang added 2 commits June 12, 2017 23:57

CleanUp ParamName

6fb7eb5

ReName to binding

7e85a56

Remove extra array layer

9b9e032

QilongTang changed the title ~~[WIP] Serialize sessionTraceData to Bindings~~ Serialize sessionTraceData to Bindings Jun 13, 2017

QilongTang removed the WIP label Jun 13, 2017

QilongTang merged commit ae3a6f5 into master Jun 13, 2017

QilongTang deleted the Serialize-sessionTraceData branch June 13, 2017 14:55

Conversation

QilongTang commented Jun 8, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Purpose

Declarations

Reviewers

FYIs

Uh oh!

mjkkirschner Jun 8, 2017

Choose a reason for hiding this comment

Uh oh!

QilongTang Jun 8, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mjkkirschner Jun 8, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

QilongTang commented Jun 12, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mjkkirschner commented Jun 12, 2017

Uh oh!

gregmarr commented Jun 12, 2017

Uh oh!

gregmarr commented Jun 12, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mjkkirschner commented Jun 12, 2017

Uh oh!

QilongTang commented Jun 12, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

QilongTang commented Jun 13, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gregmarr commented Jun 13, 2017

Uh oh!

QilongTang commented Jun 13, 2017

Uh oh!

gregmarr commented Jun 13, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

QilongTang commented Jun 13, 2017

Uh oh!

gregmarr commented Jun 13, 2017

Uh oh!

QilongTang commented Jun 13, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

QilongTang commented Jun 8, 2017 •

edited

Loading

QilongTang Jun 8, 2017 •

edited

Loading

mjkkirschner Jun 8, 2017 •

edited

Loading

QilongTang commented Jun 12, 2017 •

edited

Loading

gregmarr commented Jun 12, 2017 •

edited

Loading

QilongTang commented Jun 12, 2017 •

edited

Loading

QilongTang commented Jun 13, 2017 •

edited

Loading

gregmarr commented Jun 13, 2017 •

edited

Loading