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

Serialiser_Engine: Fix serialiser crash on failed #3086

Merged

Conversation

adecler
Copy link
Member

@adecler adecler commented Jun 14, 2023

Issues addressed by this PR

Closes #3066

As detailed in the issue, the BHoM UI was crashing on the deserialisation of a specific dataset. It appears that the 'failed' boolean that is passes up from properties to parents was used to force a different deserialisation behaviour inside the SetProperties method.

  • The issue is that that boolean is never reset between properties. This means one incorrect property would trigger a failing behaviour on all the following ones.
  • Deserialisation is also capable of restoring partial objects by saving the remaining properties inside CustomData. Those objects were still marked as 'failed' though causnig the whole chain of parents to come back as Custom objects.

As the only place where the 'failed' boolean is used for a conditional check is inside the SetProperty method (where it is incorrectly used), I would recommend to get rid of that boolean altogether. We agreed with @FraserGreenroyd to do this in two phases:

Test files

  • Make sure the versioning check is passing
  • Test on older versioning datasets (at least a year coverage)
  • Test on datasets found in the BHoM folder

@adecler adecler added type:bug Error or unexpected behaviour status:WIP PR in progress and still in draft, not ready for formal review labels Jun 14, 2023
@adecler adecler self-assigned this Jun 14, 2023
@adecler adecler requested a review from FraserGreenroyd as a code owner June 14, 2023 13:22
@adecler
Copy link
Member Author

adecler commented Jun 14, 2023

@BHoMBot check versioning

@bhombot-ci
Copy link

bhombot-ci bot commented Jun 14, 2023

@adecler to confirm, the following actions are now queued:

  • check versioning

There are 14 requests in the queue ahead of you.

@FraserGreenroyd
Copy link
Contributor

@BHoMBot check compliance
@BHoMBot check serialisation

@bhombot-ci
Copy link

bhombot-ci bot commented Jun 14, 2023

@FraserGreenroyd to confirm, the following actions are now queued:

  • check code-compliance
  • check documentation-compliance
  • check project-compliance
  • check branch-compliance
  • check dataset-compliance
  • check copyright-compliance
  • check serialisation

@FraserGreenroyd
Copy link
Contributor

@BHoMBot check core
@BHoMBot check unit-tests
@BHoMBot check installer
@BHoMBot check null-handling

@bhombot-ci
Copy link

bhombot-ci bot commented Jun 14, 2023

@FraserGreenroyd to confirm, the following actions are now queued:

  • check core
  • check unit-tests
  • check installer
  • check null-handling

Copy link
Contributor

@FraserGreenroyd FraserGreenroyd left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have reviewed the code and it's only deleted lines which is always nice to tidy up 😄

I have then performed the following actions on this branch:

  • Deserialised Test Sets for Versioning from version 3.3 to 6.1 for objects and methods
    • Some failures are noted during this deserialisation, however, they are either known issues as a result of the new serialisation framework, or not related to this particular work
    • Despite failures, every single test set JSON file deserialised without crashing Rhino/Grasshopper
    • Objects were returned where possible
  • Deserialised every dataset housed within BHoM_Datasets
  • All datasets deserialised without crashing Rhino/Grasshopper
  • Some errors in deserialisation as a result of failing to convert types for objects I do not currently have compiled on my machine, with all other datasets deserialising correctly

In addition, the dataset issue described in the #3066 has also stopped crashing Rhino entirely and deserialises to a dataset but with errors reported as expected.

Thus, I am happy to merge this for wider testing during our 6.2 beta close out sprint this sprint, but this improves the behaviour discussed 😄

@FraserGreenroyd FraserGreenroyd removed the status:WIP PR in progress and still in draft, not ready for formal review label Jun 14, 2023
@bhombot-ci
Copy link

bhombot-ci bot commented Jun 14, 2023

FAO: @FraserGreenroyd
@FraserGreenroyd is seeking dispensation on this Pull Request to skip a required check. Please can you provide authorisation for the check to be skipped, or provide assistance as appropriate.

The check they wish to have dispensation on is unit-tests.

If you are providing dispensation on this occasion, please reply with:

@BHoMBot this is a DevOps instruction. I am authorising dispensation to be granted on check ref. 14265547610

@FraserGreenroyd
Copy link
Contributor

@BHoMBot this is a DevOps instruction. I am authorising dispensation to be granted on check ref. 14265547610

@bhombot-ci
Copy link

bhombot-ci bot commented Jun 14, 2023

@FraserGreenroyd I have now provided a passing check on reference 14265547610 as requested.

@FraserGreenroyd
Copy link
Contributor

@BHoMBot check ready-to-merge

@bhombot-ci
Copy link

bhombot-ci bot commented Jun 14, 2023

@FraserGreenroyd to confirm, the following actions are now queued:

  • check ready-to-merge

@FraserGreenroyd FraserGreenroyd merged commit b9eb2de into develop Jun 14, 2023
@FraserGreenroyd FraserGreenroyd deleted the Serialiser_Engine-#3066-FixSerialiserCrashOnFailed branch June 14, 2023 20:21
@FraserGreenroyd FraserGreenroyd changed the title Fix serialiser crash on failed Serialiser_Engine: Fix serialiser crash on failed Jun 15, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type:bug Error or unexpected behaviour
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Serialiser_Engine: Deserialising lists throws massive errors if things fail
2 participants