Open Bug 1672646 Opened 4 years ago Updated 10 months ago

Merge IndexCountRequestOp::DoDatabaseWork and ObjectStoreCountRequestOp::DoDatabaseWork

Categories

(Core :: Storage: IndexedDB, task)

task

Tracking

()

People

(Reporter: sg, Unassigned)

Details

(Keywords: good-first-bug)

Attachments

(2 files)

IndexCountRequestOp::DoDatabaseWork and ObjectStoreCountRequestOp::DoDatabaseWork do almost the same, just the query is different. A common function could be extracted from the two functions.

It might even possible to extract a common class template from the whole classes, but maybe that introduces too much complexity.

I'd like to take up this task if ya'll don't mind.

Also for this common function, to what class/object would it belong to? I'm new here so I'm not yet very familiar with the code base.

You can add the function to NormalTransactionOp class besides the ObjectStoreHasIndexes function.

Is there anything specific ya'll looking for in the method? Like should it take an input parameter as to what type of object is being used to execute DoDatabaseWork, if so I was thinking maybe using a String that can either be "IndexCountRequestOp::DoDatabaseWork" or ("ObjectStoreCountRequestOp::DoDatabaseWork" to be used to return either one of the two nsresults. However the problem I've been having with this approach is that I would need to create an instance of IndexCountRequestOp or ObjectStoreCountRequestOp, which I still don't know how to do due to still being new to this.

So we have two methods:

nsresult ObjectStoreCountRequestOp::DoDatabaseWork(
    DatabaseConnection* aConnection) {
  MOZ_ASSERT(aConnection);
  aConnection->AssertIsOnConnectionThread();

  AUTO_PROFILER_LABEL("ObjectStoreCountRequestOp::DoDatabaseWork", DOM);

  const auto keyRangeClause = MaybeGetBindingClauseForKeyRange(
      mParams.optionalKeyRange(), kColumnNameKey);

  IDB_TRY_INSPECT(
      const auto& maybeStmt,
      aConnection->BorrowAndExecuteSingleStepStatement(
          "SELECT count(*) "
          "FROM object_data "
          "WHERE object_store_id = :"_ns +
              kStmtParamNameObjectStoreId + keyRangeClause,
          [&params = mParams](auto& stmt) -> mozilla::Result<Ok, nsresult> {
            IDB_TRY(stmt.BindInt64ByName(kStmtParamNameObjectStoreId,
                                         params.objectStoreId()));

            if (params.optionalKeyRange().isSome()) {
              IDB_TRY(BindKeyRangeToStatement(params.optionalKeyRange().ref(),
                                              &stmt));
            }

            return Ok{};
          }));

  IDB_TRY(OkIf(maybeStmt.isSome()), NS_ERROR_DOM_INDEXEDDB_UNKNOWN_ERR,
          [](const auto) {
            // XXX Why do we have an assertion here, but not at most other
            // places using IDB_REPORT_INTERNAL_ERR(_LAMBDA)?
            MOZ_ASSERT(false, "This should never be possible!");
            IDB_REPORT_INTERNAL_ERR();
          });

  const auto& stmt = *maybeStmt;

  const int64_t count = stmt->AsInt64(0);
  IDB_TRY(OkIf(count >= 0), NS_ERROR_DOM_INDEXEDDB_UNKNOWN_ERR, [](const auto) {
    // XXX Why do we have an assertion here, but not at most other places using
    // IDB_REPORT_INTERNAL_ERR(_LAMBDA)?
    MOZ_ASSERT(false, "This should never be possible!");
    IDB_REPORT_INTERNAL_ERR();
  });

  mResponse.count() = count;

  return NS_OK;
}

nsresult IndexCountRequestOp::DoDatabaseWork(DatabaseConnection* aConnection) {
  MOZ_ASSERT(aConnection);
  aConnection->AssertIsOnConnectionThread();

  AUTO_PROFILER_LABEL("IndexCountRequestOp::DoDatabaseWork", DOM);

  const auto indexTable = mMetadata->mCommonMetadata.unique()
                              ? "unique_index_data "_ns
                              : "index_data "_ns;

  const auto keyRangeClause = MaybeGetBindingClauseForKeyRange(
      mParams.optionalKeyRange(), kColumnNameValue);

  IDB_TRY_INSPECT(
      const auto& maybeStmt,
      aConnection->BorrowAndExecuteSingleStepStatement(
          "SELECT count(*) "
          "FROM "_ns +
              indexTable + "WHERE index_id = :"_ns + kStmtParamNameIndexId +
              keyRangeClause,
          [&self = *this](auto& stmt) -> mozilla::Result<Ok, nsresult> {
            IDB_TRY(stmt.BindInt64ByName(kStmtParamNameIndexId,
                                         self.mMetadata->mCommonMetadata.id()));

            if (self.mParams.optionalKeyRange().isSome()) {
              IDB_TRY(BindKeyRangeToStatement(
                  self.mParams.optionalKeyRange().ref(), &stmt));
            }

            return Ok{};
          }));

  IDB_TRY(OkIf(maybeStmt.isSome()), NS_ERROR_DOM_INDEXEDDB_UNKNOWN_ERR,
          [](const auto) {
            // XXX Why do we have an assertion here, but not at most other
            // places using IDB_REPORT_INTERNAL_ERR(_LAMBDA)?
            MOZ_ASSERT(false, "This should never be possible!");
            IDB_REPORT_INTERNAL_ERR();
          });

  const auto& stmt = *maybeStmt;

  const int64_t count = stmt->AsInt64(0);
  IDB_TRY(OkIf(count >= 0), NS_ERROR_DOM_INDEXEDDB_UNKNOWN_ERR, [](const auto) {
    // XXX Why do we have an assertion here, but not at most other places using
    // IDB_REPORT_INTERNAL_ERR(_LAMBDA)?
    MOZ_ASSERT(false, "This should never be possible!");
    IDB_REPORT_INTERNAL_ERR();
  });

  mResponse.count() = count;

  return NS_OK;
}

These parts are exactly the same:

  const auto keyRangeClause = MaybeGetBindingClauseForKeyRange(
      mParams.optionalKeyRange(), kColumnNameValue);
  IDB_TRY(OkIf(maybeStmt.isSome()), NS_ERROR_DOM_INDEXEDDB_UNKNOWN_ERR,
          [](const auto) {
            // XXX Why do we have an assertion here, but not at most other
            // places using IDB_REPORT_INTERNAL_ERR(_LAMBDA)?
            MOZ_ASSERT(false, "This should never be possible!");
            IDB_REPORT_INTERNAL_ERR();
          });

  const auto& stmt = *maybeStmt;

  const int64_t count = stmt->AsInt64(0);
  IDB_TRY(OkIf(count >= 0), NS_ERROR_DOM_INDEXEDDB_UNKNOWN_ERR, [](const auto) {
    // XXX Why do we have an assertion here, but not at most other places using
    // IDB_REPORT_INTERNAL_ERR(_LAMBDA)?
    MOZ_ASSERT(false, "This should never be possible!");
    IDB_REPORT_INTERNAL_ERR();
  });

So they can be easily extracted to a helper function and I think the function should return the count as Result<int64_t, nsresult.
One of the arguments for the function needs to be DatabaseConnection* aConnection for sure.

Then we need to extract the query which is a bit harder. Let me know if this looks ok to you so far.

I've been working on this and have been having problem with extracting a common mParams value. I'm trying to do so by using a ternary operator as so

const ObjectStoreCountParams oscParams;
const IndexCountParams icParams;
const auto mParams = (requestType == "ObjectStoreCountRequestOp") ? oscParams : icParams;

however I'm getting a compile time error that states there are incompatible types when it comes to the ? operator. Any one have any clue as to what I did wrong here or how to fix this error?

Ok so I've written the helper function for the most part, now I'm just confused as how to return the count

nsresult NormalTransactionOp::CommonCountRequestOp(DatabaseConnection* aConnection, RequestParams&& aParams){

  MOZ_ASSERT(aConnection);
  aConnection->AssertIsOnConnectionThread();

  const SafeRefPtr<FullIndexMetadata> mMetadata;

  if(aParams.type() == RequestParams::TObjectStoreCountParams){
    AUTO_PROFILER_LABEL("ObjectStoreCountRequestOp::DoDatabaseWork", DOM);
  }else{
    AUTO_PROFILER_LABEL("IndexCountRequestOp::DoDatabaseWork", DOM);
  }

  const auto indexTable = mMetadata->mCommonMetadata.unique()
                              ? "unique_index_data "_ns
                              : "index_data "_ns;


  IndexCountParams icParams;
  ObjectStoreCountParams oscParams;

  auto keyRangeClause = MaybeGetBindingClauseForKeyRange((aParams.type() == RequestParams::TObjectStoreCountParams) ? oscParams.optionalKeyRange() : icParams.optionalKeyRange(), kColumnNameKey);

  IDB_TRY_INSPECT(
      const auto& maybeStmt,
      (aParams.type() == RequestParams::TObjectStoreCountParams) ? aConnection->BorrowAndExecuteSingleStepStatement(
          "SELECT count(*) "
          "FROM object_data "
          "WHERE object_store_id = :"_ns +
              kStmtParamNameObjectStoreId + keyRangeClause,
          [&params = icParams](auto& stmt) -> mozilla::Result<Ok, nsresult> {
            IDB_TRY(stmt.BindInt64ByName(kStmtParamNameObjectStoreId,
                                         params.objectStoreId()));

            if (params.optionalKeyRange().isSome()) {
              IDB_TRY(BindKeyRangeToStatement(params.optionalKeyRange().ref(),
                                              &stmt));
            }

            return Ok{};
          })
      : aConnection->BorrowAndExecuteSingleStepStatement(
          "SELECT count(*) "
          "FROM "_ns +
              indexTable + "WHERE index_id = :"_ns + kStmtParamNameIndexId +
              keyRangeClause,
          [&params = icParams, &metadata = mMetadata](auto& stmt) -> mozilla::Result<Ok, nsresult> {
            IDB_TRY(stmt.BindInt64ByName(kStmtParamNameIndexId,
                                         metadata->mCommonMetadata.id()));

            if (params.optionalKeyRange().isSome()) {
              IDB_TRY(BindKeyRangeToStatement(
                  params.optionalKeyRange().ref(), &stmt));
            }

            return Ok{};
          }));

  IDB_TRY(OkIf(maybeStmt.isSome()), NS_ERROR_DOM_INDEXEDDB_UNKNOWN_ERR,
          [](const auto) {
            // XXX Why do we have an assertion here, but not at most other
            // places using IDB_REPORT_INTERNAL_ERR(_LAMBDA)?
            MOZ_ASSERT(false, "This should never be possible!");
            IDB_REPORT_INTERNAL_ERR();
          });

  const auto& stmt = *maybeStmt;

  const int64_t count = stmt->AsInt64(0);
  IDB_TRY(OkIf(count >= 0), NS_ERROR_DOM_INDEXEDDB_UNKNOWN_ERR, [](const auto) {
    // XXX Why do we have an assertion here, but not at most other places using
    // IDB_REPORT_INTERNAL_ERR(_LAMBDA)?
    MOZ_ASSERT(false, "This should never be possible!");
    IDB_REPORT_INTERNAL_ERR();
  });


  return NS_OK;

}

Can you translate that into a diff/patch ?
Thanks.

Is there documentation that would tell me how to do that? This is my first bug and would like to know how to do so. Also how would I return the count for the method I wrote. I know that IndexCountRequestOp::DoDatabaseWork and ObjectStoreCountRequestOp::DoDatabaseWork use mResponse to set the count but NormalTransactionOp has no such variable.

Also forgot to ask but can I be assigned to this task?

Sure, I assigned the bug to you.

Regarding making a patch, I don't know how you downloaded the source code. If you used Mercurial (hg) for that, then you can just run hg diff >patch.diff and attach the file to this bug.

A more advanced option is to use Phabricator, but you don't have to worry about that for now.

Assignee: nobody → nblanque
Status: NEW → ASSIGNED

Here is that patch file, hope it helps.

Just put in another patch file, not sure what class CountRequestOp was doing there, probably forgot to delete it from something I was previously trying a month ago.

This good-first-bug hasn't had any activity for 2 months, it is automatically unassigned.
For more information, please visit auto_nag documentation.

Assignee: nblanque → nobody
Status: ASSIGNED → NEW

Hi Nathaniel, I fear we overlooked that patch for a while, sorry. Jan, can you take a look once back?

Assignee: nobody → nblanque
Flags: needinfo?(nblanque)
Flags: needinfo?(jvarga)

This good-first-bug hasn't had any activity for 2 months, it is automatically unassigned.
For more information, please visit auto_nag documentation.

Assignee: nblanque → nobody

Hi, just wanted to know if the patch solved the issue>

Flags: needinfo?(nblanque)

Sorry for really long delay. I quickly checked the patch and it seems there's still work to be done.

Flags: needinfo?(jvarga)
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: