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.

Sign up for GitHub

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

Jump to bottom

Adopt the exception unification changes #3194

Merged
merged 12 commits into from
Feb 28, 2023
Merged

Adopt the exception unification changes #3194

merged 12 commits into from
Feb 28, 2023

Conversation

nirinchev
Copy link
Member

@nirinchev nirinchev commented Jan 20, 2023 •
edited
Loading

Description

Fixes #2796

TODO

  • Changelog entry
  • Tests (if applicable)

@cla-bot cla-bot bot added the cla: yes label Jan 20, 2023
github-advanced-security[bot]
github-advanced-security bot found potential problems Jan 20, 2023
View reviewed changes
Realm/Realm/Exceptions/RealmClassLacksPrimaryKeyException.cs
@@ -25,7 +25,7 @@
{
internal RealmClassLacksPrimaryKeyException(string message) : base(message)
{
HelpLink = "https://docs.mongodb.com/realm/dotnet/objects/#primary-key";
HelpLink = "https://www.mongodb.com/docs/realm/sdk/dotnet/model-data/define-object-model/#primary-key";

Check warning

Code scanning / CodeQL

Virtual call in constructor or destructor

Avoid virtual calls in a constructor or destructor.
Realm/Realm/Exceptions/RealmDuplicatePrimaryKeyValueException.cs
@@ -25,7 +25,7 @@
{
internal RealmDuplicatePrimaryKeyValueException(string message) : base(message)
{
HelpLink = "https://docs.mongodb.com/realm/dotnet/objects/#primary-key";
HelpLink = "https://www.mongodb.com/docs/realm/sdk/dotnet/model-data/define-object-model/#primary-key";

Check warning

Code scanning / CodeQL

Virtual call in constructor or destructor

Avoid virtual calls in a constructor or destructor.
Realm/Realm/Handles/SharedRealmHandle.cs
@@ -623,7 +623,7 @@

public ObjectHandle CreateObject(TableKey tableKey)
{
var result = NativeMethods.create_object(this, tableKey.Value, out NativeException ex);
var result = NativeMethods.create_object(this, tableKey.Value, out var ex);

Check notice

Code scanning / CodeQL

Calls to unmanaged code

Replace this call with a call to managed code if possible.
@nirinchev nirinchev requested review from LaPeste and fealebenpae and removed request for LaPeste January 25, 2023 12:28
@nirinchev nirinchev marked this pull request as ready for review January 25, 2023 12:28
@coveralls
Copy link

coveralls commented Jan 27, 2023 •
edited
Loading

Pull Request Test Coverage Report for Build 4284488053

  • 24 of 26 (92.31%) changed or added relevant lines in 7 files are covered.
  • 10 unchanged lines in 3 files lost coverage.
  • Overall coverage increased (+0.3%) to 81.793%
Changes Missing Coverage Covered Lines Changed/Added Lines %
Realm/Realm/Exceptions/RealmException.cs 6 8 75.0%
Files with Coverage Reduction New Missed Lines %
Realm/Realm/Exceptions/RealmException.cs 3 78.38%
Realm/Realm/Handles/SessionHandle.cs 3 81.22%
Realm/Realm/Extensions/TaskExtensions.cs 4 75.56%
Totals Coverage Status
Change from base Build 4184333833: 0.3%
Covered Lines: 5956
Relevant Lines: 7175
💛 - Coveralls

papafe
papafe reviewed Jan 27, 2023
View reviewed changes
Realm/Realm/Exceptions/RealmExceptionCodes.cs
RLM_ERR_CALLBACK = 1000000,
RLM_ERR_UNKNOWN = 2000000,

RowDetached = 1000004000,
Copy link
Contributor

@papafe papafe Jan 27, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Why these don't have the same casing?

Copy link
Member Author

Choose a reason for hiding this comment

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

The RLM_ERR... ones are coming from Core. The PascalCased ones are custom codes that we define. I'll add a comment to make sure it's clear.

fealebenpae
fealebenpae reviewed Jan 31, 2023
View reviewed changes
Copy link
Member

@fealebenpae fealebenpae left a comment

Choose a reason for hiding this comment

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

LGTM. I like the added round tripping of client reset managed exceptions.

LaPeste
LaPeste reviewed Feb 1, 2023
View reviewed changes
wrappers/src/dictionary_cs.cpp
Comment on lines +34 to +40
if (value.is_null() && !is_nullable(dict.get_type())) {
throw NotNullable("Attempted to add null to a dictionary of required values");
}

if (!value.is_null() && dict.get_type() != PropertyType::Mixed && to_capi(dict.get_type()) != value.type) {
throw PropertyTypeMismatchException(to_string(dict.get_type()), to_string(value.type));
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
if (value.is_null() && !is_nullable(dict.get_type())) {
throw NotNullable("Attempted to add null to a dictionary of required values");
}
if (!value.is_null() && dict.get_type() != PropertyType::Mixed && to_capi(dict.get_type()) != value.type) {
throw PropertyTypeMismatchException(to_string(dict.get_type()), to_string(value.type));
}
if (value.is_null()) {
if (!is_nullable(dict.get_type())) {
throw NotNullable("Attempted to add null to a dictionary of required values");
}
}
else {
if (dict.get_type() != PropertyType::Mixed && to_capi(dict.get_type()) != value.type) {
throw PropertyTypeMismatchException(to_string(dict.get_type()), to_string(value.type));
}
}

Wouldn't it be better to avoid to repeat is_null()?

LaPeste
LaPeste approved these changes Feb 1, 2023
View reviewed changes
Copy link
Contributor

@LaPeste LaPeste left a comment

Choose a reason for hiding this comment

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

Overall it makes sense to me. I just left a cosmetic question.

For the tests there is just a timeout on Maui.Android. So all looks good on that front too. Just don't forget to add an entry in the changelog.

As a side note: I know it's not our decision, it's core's. But, I'm not sure ErrorCodes::Error::OK is a better choice than ...::NoError 😅

@nirinchev nirinchev merged commit 1affe31 into main Feb 28, 2023
@nirinchev nirinchev deleted the ni/exception-unification branch February 28, 2023 16:02
nirinchev added a commit that referenced this pull request Mar 2, 2023
@nirinchev
Merge branch 'main' into ac/overhaul-metrics
0edadcd 
* main:
  Add a concurrency setting to pr workflow (#3260)
  Adopt the exception unification changes (#3194)
  Expose options to control sync timeouts (#3224)
  Use BsonSerializer.RegisterProvider (#3226)
  Remove survey ad (#3229)
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Mar 14, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@papafe papafe papafe left review comments

@fealebenpae fealebenpae fealebenpae left review comments

@LaPeste LaPeste LaPeste approved these changes

Assignees

@nirinchev nirinchev

Labels
cla: yes
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Implement Unify Core Error Handling
5 participants
@nirinchev @coveralls @fealebenpae @LaPeste @papafe