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

Remove dropped ledger columns from Edit Data columns #1691

Merged

Conversation

nofield
Copy link
Contributor

@nofield nofield commented Sep 15, 2022

Dropped ledger columns should not be exposed to the Edit Data view, because the columns are non-editable and soft-deleted (marked as dropped and hidden, and removed from all views unless the user explicitly requests them). This PR filters dropped columns out from the list returned by GetObjectMetadata so it's never exposed in Edit Data.

Testing

Manual verification. I attempted to create a unit test for this specific scenario, but it got tricky with IsDroppedLedgerColumn not being exposed in DbColumn (only SMO Column), and trying to set specific properties in a column passed to GetObjectMetadata. If you have suggestions for how to work around these issues I'd be happy to write an automated test.

Table:
image

Edit Data does not expose the dropped column:
image

@shueybubbles
Copy link
Contributor

        for (int i = 0; i < smoResult.Columns.Count; i++)

I highly recommend using smoREsult.Columns.ClearAndInitialize, passing a filter to exclude the undesirable rows on the server side and also passing the names of the properties you reference.
Otherwise perf is going to be really bad for objects with many columns.


Refers to: src/Microsoft.SqlTools.ServiceLayer/EditData/SmoEditMetadataFactory.cs:98 in 21470b0. [](commit_id = 21470b0, deletion_comment = False)

@shueybubbles
Copy link
Contributor

        for (int i = 0; i < smoResult.Columns.Count; i++)

https://github.com/microsoft/sqlmanagementobjects/wiki/Refresh-vs-ClearAndInitialize


In reply to: 1249850114


Refers to: src/Microsoft.SqlTools.ServiceLayer/EditData/SmoEditMetadataFactory.cs:98 in 21470b0. [](commit_id = 21470b0, deletion_comment = False)

Copy link
Contributor

@shueybubbles shueybubbles left a comment

Choose a reason for hiding this comment

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

🕐

@Charles-Gagnon
Copy link
Contributor

Charles-Gagnon commented Sep 16, 2022

Thanks, I created a separate issue to track the perf improvement since this PR isn't changing anything there.

#1697

@smartguest FYI

@nofield
Copy link
Contributor Author

nofield commented Sep 19, 2022

        for (int i = 0; i < smoResult.Columns.Count; i++)

I highly recommend using smoREsult.Columns.ClearAndInitialize, passing a filter to exclude the undesirable rows on the server side and also passing the names of the properties you reference. Otherwise perf is going to be really bad for objects with many columns.

Refers to: src/Microsoft.SqlTools.ServiceLayer/EditData/SmoEditMetadataFactory.cs:98 in 21470b0. [](commit_id = 21470b0, deletion_comment = False)

I moved the dropped ledger column filtering into ClearAndInitialize as recommended, thanks @shueybubbles. @Charles-Gagnon, David was saying that the dropped ledger column filtering as it was would be a perf impact, better to handle the filtering in SMO itself.

I'm not sure if I'm now duplicating refresh calls on the columns, though, since there's both a call to ClearAndInitialize on the Columns and the Refresh() on the entire smoResult object. I looked into if the call to Refresh() could be replaced, but the 'State' property that's being checked against on line 95 is being set during that call.

// Filter out dropped ledger columns from the list of columns to be returned
//
smoResult.Columns.ClearAndInitialize("[(@IsDroppedLedgerColumn=0)]", new [] {nameof(Column.IsDroppedLedgerColumn), nameof(Column.DataType)});

// A bug in SMO makes it necessary to call refresh to attain certain properties (such as IsMemoryOptimized)
Copy link
Contributor

Choose a reason for hiding this comment

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

// A bug in SMO

what bug is that?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, I'm not sure what that's referring to.

@shueybubbles
Copy link
Contributor

                    : FromSqlScript.UnwrapLiteral(smoColumn.DefaultConstraint.Text);

Add nameof(Column.DefaultConstraintName) to the property list in ClearAndInitialize, and don't try to fetch DefaultConstraint unless smoColumn.DefaultConstraintName is non-empty.


Refers to: src/Microsoft.SqlTools.ServiceLayer/EditData/SmoEditMetadataFactory.cs:112 in b6e5707. [](commit_id = b6e5707, deletion_comment = False)

Copy link
Contributor

@shueybubbles shueybubbles left a comment

Choose a reason for hiding this comment

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

:shipit:

@nofield nofield merged commit ec1bb0e into main Sep 19, 2022
@nofield nofield deleted the dev/nofield/remove-dropped-ledger-columns-from-edit-data branch September 19, 2022 23:22
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

Successfully merging this pull request may close these issues.

5 participants