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

Renaming a table loses its relationships #397

Closed
o-o00o-o opened this issue Feb 21, 2020 · 15 comments
Closed

Renaming a table loses its relationships #397

o-o00o-o opened this issue Feb 21, 2020 · 15 comments
Milestone

Comments

@o-o00o-o
Copy link

This is a biggie that caused us to have to do an emergency release this week, so think this should be fixed ASAP to save others from the same fate

We did a rename of a table and it didn't keep the relationships. I think it should be able to deal with this!

@otykier
Copy link
Collaborator

otykier commented Feb 21, 2020

When I load a model in TE and rename a table, all relationships to and from that table remain intact.

Could you provide exact steps needed to reproduce this? Also, please provide information about when the issue arises: During deployment, during serialisation to folder, etc. Thanks!

@o-o00o-o
Copy link
Author

that is wierd. When I tested earlier (literally just renaming the table and checking the dependencies) it lost all relationships. Now when I'm doing exactly the same thing it is working. This actually mirrors what happened to us as on development it did work ok for the developer but when it went through the breaking change system it lost them.

This breaking change did a Script driven rename on a bunch of columns, measures and tables. I have seen it happen on a simple rename but also I've seen it not happen. i think when we pass a load of renames through in the script it seems that it is more likely to happen, but I'm seeing it be inconsistent there also.

So, right now it is looking like an intermittent issue in 2.9.1. I'll try a few things to get a repro

@o-o00o-o
Copy link
Author

thank god for that.. I've found it. I thought I was going mad. I've been writing loop scripts, checks in my complex custom action script and finally narrowed it down.

Ok, so the issue only happens if you rename immediately after loading a model before you do anything else.

So repro by
1 open model
2 check that you have a fact table identified and that the related tables show in the dependency viewer (I've proved with a couple of different fact tables with 4 and 6 relationships respectively but not sure if it matters)
3 reopen the model (Recent Files \ 1)
4 rename the fact table (F2, add a 2 to the end or something)
5 check the dependency viewer - you will see no relationships.

Interestingly the relationships are actually still there (the Model.Relationships.Count is still the same) but they are butchered with some renamed and some not, but all of them are no longer "attached" to the table

If you now save the model the relationships are fully removed and you have an Emergency Change on your hands unless it is spotted in a PR - ours wasn't

Interestingly if you now
6. Undo
7. F2 rename in exactly the same way.
8. Check Dependency viewer, it works as normal.

Also if you miss out reopening the mode (step 3) the issue doesn't present.

So this only caused us a problem as we have an automated breaking change system that does bulk renaming/deleting on each of our releases, so the release masters just open the model and run the script which is why it affected us and not the developer while she was developing

Nice one Daniel, that was a tough one! Do I get a prize? :)

@o-o00o-o
Copy link
Author

@otykier can you repro this now and is it going to be easy to fix?

This issue had a high profile impact for us and I'm keen to ensure that it doesn't happen again.

@otykier
Copy link
Collaborator

otykier commented Feb 27, 2020

I'm still not able to repro. I've tried changing model names through a script as well, but no matter what I do, I can't get the relationships to disappear.

I'm assuming you're using folder serialisation. Can you repro this issue if you load the model from a .bim file or from a deployed database? Could you send me a screenshot of the serialisation settings used by your model? (File > Preferences > Current Model).

@o-o00o-o
Copy link
Author

o-o00o-o commented Mar 2, 2020

Yes confirmed it doesn't happen if I load a .bim file, only loading from folder serialisation

Settings are as below (Data Sources and Perspectives are also ticked but can't resize to show these in one screenshot)

image

@otykier
Copy link
Collaborator

otykier commented Mar 3, 2020

Unfortunately I'm still not able to reproduce this, but I'd really like to get to the bottom of this, as I currently have no idea what could be causing this. Can you send me a .zip of the folder structure for the model you're currently working on? If not, may I ask you to produce a minimal model that displays the same behaviour?

@o-o00o-o
Copy link
Author

o-o00o-o commented Mar 6, 2020

I wouldn't get approval to send our model out so I just built a minimal model and the same thing happens - see attached

DemoRenameIssue.zip

Simply open the model

image

, select Blah, F2, rename to Blah A

image

, and see that table dependencies have gone

image

I wondered whether it could be anything to do with our Custom Actions so I renamed the CustomActions.json file and tried it again after I took the screenshots but it made no difference, so try with no custom actions also.

otykier pushed a commit that referenced this issue Mar 9, 2020
@otykier
Copy link
Collaborator

otykier commented Mar 9, 2020

Thank you very much! This sample model finally allowed me to reproduce the issue. Issue is fixed in 2.9.4.

For those interested, this was an issue with the way the model was deserialized when saved to a folder structure AND certain objects were serialized as annotations on other objects (this is possible for perspective membership information, translations and relationships - the latter being the case here).

The way Tabular Editor 2.9.3 and earlier versions did this was to loop through all folders and subfolders, concatenating all the .json files into a single json document which was then deserialized by the AMO libs' JsonSerializer.DeserializeDatabase method. At this point, we would have a model without any perspectives, translations or relationships.

Then, Tabular Editor would traverse the entire model tree, to locate objects with the special TabularEditor_xxx annotations, and create the equivalent perspectives, translations and relationships based on the embedded JSON in the annotations. In the sample provided, consider a relationship embedded into the annotations on table 'Blah':

{
    "fromTable": "Blah",
    "fromColumn": "BlahKey",
    "toTable": "Blah2",
    "toColumn": "BlahKey"
}

This relationships is deserialized using the JsonSerializer.DeserializeObject method. However, at this point, the relationship has not yet been added to the model tree, so its FromTable/FromColumn/ToTable/ToColumn properties are all null. Yet - the TOM internally knows the names of which objects the relationship is referencing. Unfortunately, the TOM AMO has been designed such that the public properties are lazily evaluated, meaning that the references to other TOM objects are not resolved until the properties are called. This behaviour causes the bug as we have seen, since the repro steps includes changing the table name BEFORE inspecting any properties of the relationship. When we look at the relationship AFTER changing the table name, we saw that FromTable/FromColumn were both null, causing some undefined behaviour in Tabular Editor.

The solution that I implemented, was to merge the annotated objects into the full JSON document of the TOM tree, before calling JsonSerializer.DeserializeDatabase, as this ensures that the TOM fully resolves all references, leaving nothing to be lazily evaluated.

As a positive side effect, this also causes loading a model to be slightly faster, as it's now a one-step operation as far as the TOM is concerned. Also, I think this might also have resolved some issues I've seen elsewhere when scripts that change object names are executed through the command-line (and therefore before lazy evaluation of object references has happened inside the TOM).

Phew! Glad to have gotten this bug out of the way :-) Keep the bug reports coming!

@otykier otykier closed this as completed Mar 9, 2020
@marcosqlbi
Copy link

This is one of those bugs you hate and you love to catch! :)

@otykier otykier added this to the 2.9.4 milestone Mar 10, 2020
@o-o00o-o
Copy link
Author

@otykier following this I am seeing the serialisation method changing which is causing unexpected diffs.. (things we didn't change) e.g.

image

Is this expected and will be stable from now on or could this be something that could continue?

@otykier
Copy link
Collaborator

otykier commented Mar 11, 2020

From your screenshot it seems that only the sort order of the items in the embedded Json array has changed. Can you confirm that to be the case? This was actually a change made in an earlier release, to ensure that they are always ordered alphabetically by the properties.

@o-o00o-o
Copy link
Author

We jumped from 2.9.2 to 2.9.4 but we definitely released using that so strange we are only seeing the serialisation change now?

It does seem to be just changing the order although I can't see that it is doing it alphabetically? What are the properties it is using to sort and in what order? I can't see it is consistent - I'm seeing both fromTable, toTable in differring orders and also fromTable, fromColumn

@otykier
Copy link
Collaborator

otykier commented Mar 12, 2020

Okay, I see now. There was a bug introduced here, in which sorting by FromColumn no longer happens. I'll reintroduce that and provide an updated build (2.9.5) shortly, so I recommend you hold off with upgrading until that is out.

@otykier
Copy link
Collaborator

otykier commented Mar 12, 2020

2.9.5 is available now.

SergioMurru pushed a commit to SergioMurru/TabularEditor that referenced this issue Feb 13, 2023
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

No branches or pull requests

3 participants