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

Fix dac_object_t::verify_type() implementation and uses #1122

Merged
merged 9 commits into from
Dec 9, 2021

Conversation

LaurentiuCristofor
Copy link
Contributor

I could no longer find a way to trigger an assert for an invalid id value.

Instead, I found this issue where get() and delete_row() methods would report a problem with ID '0' when called with an invalid id value. This was due to the verify_type() method, which would return id '0' as a way of indicating that the id could not be found. I changed the signature of the method and its uses to eliminate this issue.

Copy link
Contributor

@senderista senderista left a comment

Choose a reason for hiding this comment

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

This is a welcome fix, but also note that there are several other uses of report_invalid_object_id() that I consider semantically invalid and confusing to users. (See https://gaiaplatform.atlassian.net/browse/GAIAPLAT-1280.) Specifically, all 4 occurrences of this pattern in production/inc/gaia/direct_access/dac_object.inc are incorrect:

    if (!dac_base_t::exists())
    {
        report_invalid_object_id(gaia::common::c_invalid_gaia_id);
    }

The invalid_object_id exception thrown with c_invalid_gaia_id implies that the user supplied 0 as the ID (which is what they will see in the error message, not the c_invalid_gaia_id symbol). But this will be thrown when they dereference any gaia_ptr that may have been constructed with an ID that was valid at the time but has since been deleted (so the locator wrapped by the gaia_ptr contains c_invalid_gaia_offset in the current snapshot, triggering the exception when the locator is dereferenced).

We can certainly address this issue in a separate PR, of course.

Copy link
Contributor

@simone-gaia simone-gaia left a comment

Choose a reason for hiding this comment

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

LGTM, but I wonder if it makes sense at all to let the user create an object with a known invalid ID. I'd rather throw an exception. If the user was not expecting an invalid ID the exception will stop execution: good. If the user was expecting an invalid ID he/she will have a try-catch.

@@ -24,13 +24,19 @@ namespace direct_access
//
// Exception class implementations.
//
invalid_object_state_internal::invalid_object_state_internal()
{
m_message = "An operation was attempted on an object that does not exist.";
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@vDonGlover : I am introducing this message to replace the "Cannot find an object with ID '0'." message, which is misleading, because this could also be thrown for objects that were initialized with valid IDs, but then were deleted.

@LaurentiuCristofor LaurentiuCristofor merged commit e79a9de into master Dec 9, 2021
@LaurentiuCristofor LaurentiuCristofor deleted the laur_dac branch December 9, 2021 01:34
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.

4 participants