-
Notifications
You must be signed in to change notification settings - Fork 1.3k
CSHARP-4040: Use AppContext switch to disable CSHARP-4040 validation #1825
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
Conversation
|
|
||
| void EnsureNoMemberMapConflicts(string elementName) | ||
| { | ||
| if (AppContext.TryGetSwitch("DisableCSharp4040Validation", out bool disableCSharp4040Validation) && disableCSharp4040Validation) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If this switch is not set this change has no effect so should be very safe.
|
|
||
| public void Dispose() | ||
| { | ||
| AppContext.SetSwitch("DisableCSharp4040Validation", false); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure this is safe if any tests run in parallel.
We might choose not to push these tests to main, just the change to BsonClassMap.cs.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good question,
@sanych-sun any creative ideas?
Also if we are turning off the switch in Dispose, should we set turn it on in the constructor?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We do not run tests in parallel, at least for now. We should make MongoClient not rely on statics before we can really run them in parallel. I suppose we either remove this workaround earlier, or will find a better way to configure this somehow via MongoClientSettings.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For now: setting this in constructor and removing in Dispose is good enough.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also if we are turning off the switch in Dispose, should we set turn it on in the constructor?
That might be too late.
What we really want to do is turn it off after ALL the tests in this file have run, but I don't know a way to do that. Turning it off after EACH test has run is the closest I can come to that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We do not run tests in parallel, at least for now
So that means we could push this test file to main if we want to. We might remove the switch long before we are able to run tests in parallel.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
or will find a better way to configure this somehow via MongoClientSettings
To do it via MongoClientSettings would require adding to the public API. Since this is for one customer only a global switch should be adequate.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What we really want to do is turn it off after ALL the tests in this file have run, but I don't know a way to do that. Turning it off after EACH test has run is the closest I can come to that.
If it's turned off after each test, but set once, does that mean that just the first test case will see the switch turned on?
I think per-class disposing can be achieved by overriding dispose in ClassFixture.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is a bit odd... we are turning the switch on once in the static constructor and turning it off 6 times in the Dispose method (because there are 6 tests).
The reason it works OK is that by the time the first test has finished running the class map has been frozen and the switch is not consulted again.
I wouldn't worry too much about any of this since we will before long remove this switch and these tests.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we are turning the switch on once in the static constructor and turning it off 6 times in the Dispose method (because there are 6 tests).
We can either set it in constructor and remove in Dispose of the test class - because test class is being created for each test, or we can use Fixture for that. Fixture is being created once for the whole test collection (test class in this case) and will be disposed only once - when all tests are done.
| cm.SetIsRootClass(true); | ||
| cm.SetDiscriminatorIsRequired(true); | ||
| cm.MapMember(x => x.TypeNames).SetShouldSerializeMethod(_ => false); | ||
| cm.SetDiscriminatorConvention(discriminatorConvention); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is only slightly different than what the user was doing in their sample code, but the end result is the same.
We can't modify the global serialization configuration the way they do, but we can simulate closely on this one class what they are doing globally.
| { | ||
| static CSharp4040SwitchTests() | ||
| { | ||
| AppContext.SetSwitch("DisableCSharp4040Validation", true); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ajcvickers thank you for suggesting this approach. I was unaware of it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Second that, nice trick.
|
|
||
| void EnsureNoMemberMapConflicts(string elementName) | ||
| { | ||
| if (AppContext.TryGetSwitch("DisableCSharp4040Validation", out bool disableCSharp4040Validation) && disableCSharp4040Validation) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we use some more meaningful name? DisableDiscriminatorFieldUniqueValidation for example.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for thinking of that. I meant to ask if anyone wanted to suggest a different name.
What do others think?
In some ways the name is not very important because we plan to remove this anyway.
I hesitate to give it too good a name that sounds like we want people to use!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would also suggest adding a namespace and a 'Switch' prefix:
Switch.MongoDB.Driver.DisableDiscriminatorFieldUniqueValidation
Looks like MS has the convention of prefixing the switches with 'Switch' string.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I used Switch.MongoDB.Driver.DisableDiscriminatorFieldConflictCheck.
The discriminator by definition is unique because there is only one. What we are checking is that it does not conflict with any members of the POCO.
| { | ||
| static CSharp4040SwitchTests() | ||
| { | ||
| AppContext.SetSwitch("DisableCSharp4040Validation", true); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Second that, nice trick.
|
|
||
| void EnsureNoMemberMapConflicts(string elementName) | ||
| { | ||
| if (AppContext.TryGetSwitch("DisableCSharp4040Validation", out bool disableCSharp4040Validation) && disableCSharp4040Validation) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would also suggest adding a namespace and a 'Switch' prefix:
Switch.MongoDB.Driver.DisableDiscriminatorFieldUniqueValidation
Looks like MS has the convention of prefixing the switches with 'Switch' string.
|
|
||
| public void Dispose() | ||
| { | ||
| AppContext.SetSwitch("DisableCSharp4040Validation", false); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good question,
@sanych-sun any creative ideas?
Also if we are turning off the switch in Dispose, should we set turn it on in the constructor?
BorisDog
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
papafe
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
sanych-sun
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
BorisDog
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
No description provided.