Skip to content

[BugFix] Handle configs which have missing options for MsSql#1580

Merged
Aniruddh25 merged 10 commits intomainfrom
handleNullOptions
Jul 17, 2023
Merged

[BugFix] Handle configs which have missing options for MsSql#1580
Aniruddh25 merged 10 commits intomainfrom
handleNullOptions

Conversation

@Aniruddh25
Copy link
Copy Markdown
Collaborator

@Aniruddh25 Aniruddh25 commented Jul 14, 2023

Why make this change?

  • With the recent refactor Overhaul of config system #1402, if options property is not specified in the config, after deserialization the Options dictionary of DataSource object is null
  • This leads to a null dereference when trying to GetTypedOptions(MsSqlOptions) here.

What is this change?

  • Make Options a nullable property since its not required as per the schema.
  • Handle all the instances of Options considering it could be null.

How was this tested?

  • Manually running dab.
  • Automated tests:
    1. Removed the options: set-session-context from the initial config used in the CLI tests to verify deserialization results in a null Options dictionary as demonstrated by the Snapshots
    2. Modified tests which create a custom config and start the engine by setting Options to null and verifying those continue to pass.
    Note: The checked in configuration file for MsSql sets the session context to true for integration tests of row level security feature. So, did not modify that file even though options is optional for MsSql.

Copy link
Copy Markdown
Contributor

@seantleonard seantleonard left a comment

Choose a reason for hiding this comment

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

Thanks for adding. Pending addressing ci/cd build error

Comment thread src/Service.Tests/TestHelper.cs
Comment thread src/Config/ObjectModel/DataSource.cs
@Aniruddh25 Aniruddh25 enabled auto-merge (squash) July 17, 2023 11:45
Copy link
Copy Markdown
Contributor

@ayush3797 ayush3797 left a comment

Choose a reason for hiding this comment

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

LGTM!

@Aniruddh25 Aniruddh25 merged commit b135913 into main Jul 17, 2023
@Aniruddh25 Aniruddh25 deleted the handleNullOptions branch July 17, 2023 11:53
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