Versioning_Engine: Add BHoM version in serialisation

[adecler]

Issues addressed by this PR

Closes #2346

This add the versioning info to each object during serialisation.

It is important to note that only version 4.1 will be able to open files from version 4.2. We've never had a good support for backward compatibility. So it was already not recommended to open a file from a version more recent than what is installed on your machine since modified objects/methods would likely break. This, however would break every single component for version 4.0 and older trying to open a file from version 4.2. Looking at BHoM analytics, we currently have around 45% of the people using version 4.0 or older.

Again, this only affects versions 4.0 and older opening files from 4.2 BUT I would not merge this PR until:

• We've properly communicated this change to our user base
• Everyone on this PR approved the change.

Test files

https://burohappold.sharepoint.com/:f:/r/sites/BHoM/02_Current/12_Scripts/02_Pull%20Request/BHoM/BHoM_Engine/Serialiser_Engine/%232346-AddVersionToJson?csf=1&web=1&e=5OxN0P

This file should open and run without problem in version 4.2 and beta 4.1. Beta 4.0 and older would fail on all components

[FraserGreenroyd]

Really minor suggestion from code review. Have not done functionality review as have been tied up with 4.1.b.1 stuff. I agree with your points though @adecler in terms of restriction to merging. Just to make sure I fully understand though, if we didn't merge this till after the 4.2 beta, the problem would still only exist for pre-4.1 betas as you've described? (i.e. waiting more betas to ensure clear comms does not jump the version affected?). This is mostly to help make sure I've fully understood the message in your comment, but also to make sure if this is not the correct understanding that adequate comms and reviewing resource are given to this PR this milestone as appropriate 😄

[adecler]

Just to make sure I fully understand though, if we didn't merge this till after the 4.2 beta, the problem would still only exist for pre-4.1 betas as you've described?

Exactly! This means the longer we wait, the less impact this PR will have. On the other hand, versioning will keep being slow until this is merged (as it starts from the first available upgrader instead of the one matching the version inside the json).

[pawelbaran]

Taken #2411 into account, I would not mind earlier release - it was never a smart thing to open a script with an older BHoM version than the script was created in. However, that is only my biased perspective, and will not be too upset if the decision is made to wait until 4.3.

[adecler]

@BHoMBot check serialisation

[bhombot-ci]

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

• serialisation

[alelom]

Taken #2411 into account, I would not mind earlier release - it was never a smart thing to open a script with an older BHoM version than the script was created in. However, that is only my biased perspective, and will not be too upset if the decision is made to wait until 4.3.

I second this - happy for this feature to be merged.

[adecler]

All to summarise the discussion we just had and clarify the scope of the testing:

• This PR breaks forward compatibility between 4.2 and versions older than 4.1 beta. So a key part of approving this PR is that you don't have an issue with introducing that specific breaking change.

• In term of testing then, here's the things you need to look for:

  • Is serialisation still working on this PR? This one is already answered by our CI/CD check on serialisation: https://github.com/BHoM/BHoM_Engine/pull/2434/checks?check_run_id=2608746375
  • Is the _bhomVersion field added to the top-level object in the serialised json for all objects ?
  • Can all non-upgraded object serialised with this PR be deserialised with beta versions 4.1 and earlier 4.2 versions ? Obviously objects types that have been versioned would not work regardless of this PR.

[rolyhudson]

Thanks for further detail @adecler. I also think we should merge this asap.

Following on from my question earlier I realise now why I was uncertain if this was enough testing.
On reviewing this PR I followed the steps below.

I install last nights alpha

• Open the test file
• I see the current version and version used to make the file are both 4.2
  
• No version is added to the json string of objects created within the script
• So all works as I am expecting👍

I build this branch.

• re open up the test file.
• Now the current version is showing as 4.1 and version used to build 4.2
  
• This confuses me I am expecting 4.2 for both.
• I note in Versioning AssemblyInfo.cs and elsewhere [assembly: AssemblyFileVersion("4.1.0.0")]
• Version is captured as 4.1 in the json string but I know this should be 4.2.
  

Fairly certain this is just a case of the PR needing an update to master to correct the AssemblyFileVersion.
Or perhaps this is ignorance on my part and you can explain my misunderstanding?

[adecler]

@rolyhudson , Yes, I was just noticing the same thing 😄 . I have rebased the PR so all is now on 4.2.

[adecler]

@BHoMBot check serialisation

[bhombot-ci]

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

• serialisation

[adecler]

I have added a set of test files here to make sure that every single object/method serialised with this PR can still be opened in older 4.2 and 4.1 versions. Here's how to do the test:

• Download Versioning_Toolkit-42.gh and the two json files
• Make sure you are either on the current master (not this PR) or on 4.1 beta.
• Open Versioning_Toolkit-42 and change the link to the json files in the top left panel.
• Make sure that all objects are deserialising correctly (4.1 will obviously have problem with versioned items but should be fine with the rest).

[rolyhudson]

I've re-run the original test script and the newer serialisation tests and happy with the results.
It should also be reviewed by others before merging.

[adecler]

@BHoMBot check required

[adecler]

I have updated the code to keep versioning deeper in the serialisation. The orignal version where the _bhomVersion field was removed in the FromBson was perfect for version 4.1 (as it was not doing anything with _bhomVersion anyways) but a bit too limited for this PR. I have rerun all the test above and added one specific to non-BHoM objects as top level objects. All is still working fine after the last commit but I want to investigate a bit further before giving this the green light.

[adecler]

I have done further tests this morning and all look good for me.

I have also checked that versioning is getting access to the version stored in Json and it does:

[adecler]

@BHoMBot check required

[bhombot-ci]

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

• code-compliance
• documentation-compliance
• project-compliance
• core
• null-handling
• serialisation
• installer
• versioning

[bhombot-ci]

FAO: @FraserGreenroyd
@adecler 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 code-compliance.

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

@BHoMBot this is a CI/CD instruction. I am authorising dispensation to be granted on check ref. 2635589758

[adecler]

@BHoMBot check copyright-compliance
@BHoMBot check dataset-compliance

[bhombot-ci]

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

• copyright-compliance
• dataset-compliance

[adecler]

@FraserGreenroyd, the bot is trying to enforce non-static methods to be extensions methods. So probably something we want to fix.

[rolyhudson]

Re tested following changes still happy to approve.

[adecler]

@BHoMBot check ready-to-merge

[bhombot-ci]

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

• ready-to-merge

There are 11 requests in the queue ahead of you.

[bhombot-ci]

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

• ready-to-merge

[adecler]

Can everyone confirm that they are happy with merging this PR ?

[FraserGreenroyd]

I'm happy from my review of this @adecler