Skip to content

Conversation

@Aniruddh25
Copy link
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
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

@Aniruddh25 Aniruddh25 enabled auto-merge (squash) July 17, 2023 11:45
Copy link
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.

6 participants