SQL: change Statement::Column* signatures [chromium/src : main]
Evan Stade (Gerrit)
Evan Stade added 1 comment![]( "Open in Gerrit")
Greg ptal //sql
Colin ptal (mechanical) changes in //components
Note that the CheckCanReadColumn checks did catch one error in autofill db migration code. (See crbug.com/449188903) Unfortunately this didn't get turned into a crbug issue until Oct 3 which was after M142 branched and these DCHECKs became CHECKs. But that issue is being addressed separately so it seems like we can go ahead and make the planned updates.
Related details
- Colin Blundell
- Greg Thompson
- Jihad Hanna
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Colin Blundell (Gerrit)
Colin Blundell voted and added 1 comment![]( "Open in Gerrit")
Votes added by Colin Blundell
| Code-Review | +1 |
1 comment
Related details
- Evan Stade
- Greg Thompson
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Greg Thompson (Gerrit)
Greg Thompson added 12 comments![]( "Open in Gerrit")
mainly lgtm w/ cleanups below. thanks for this. the API is much cleaner this way.
if (!bitmap_data_blob.empty()) {this value is only needed within the `if`, so:
```
if (auto blob = ...; !blob.empty()) {
...
}
```
results.push_back(std::move(encrypted_password));let's just inline `s.ColumnBlobAsString(0)` here
results.push_back(std::move(encrypted_note));`s.ColumnBlobAsString(0)` inline
if (entry_ok) {inline `!service.empty()` here
CHECK_EQ(last_sqlite_result_code_.value(), SqliteResultCode::kRow)this will cause a crash within the call to `value()` if `last_sqlite_result_code_` doesn't hold a value. this might be a bit confusing to diagnose if it happens. wdyt about changing this to something like `.value_or(SqliteResultCode::kOk)` so that the `CHECK_EQ` will fire in all cases?
<< " reading from columns must happen after Step() is called and "
"returns true";strings like this take up space in the binary for little benefit. please move this into a comment that immediately precedes the `CHECK_EQ`. it will be trivial to find if this `CHECK` is ever hit.
CHECK_GE(column_index, 0, base::NotFatalUntil::M142);please retain this check. if you don't think it's necessary: why?
const void* result_buffer = sqlite3_column_blob(ref_->stmt(), column_index);while you're here, please swap the order of these two lines as per https://www.sqlite.org/c3ref/column_blob.html: "you should call sqlite3_column_text(), sqlite3_column_blob(), or sqlite3_column_text16() first to force the result into the desired format, then invoke sqlite3_column_bytes() or sqlite3_column_bytes16() to find the size of the result."
CheckCanReadColumn(column_index);
const void* result_buffer = sqlite3_column_blob(ref_->stmt(), column_index);
int size = sqlite3_column_bytes(ref_->stmt(), column_index);
if (result_buffer && size > 0) {
return std::string(reinterpret_cast<const char*>(result_buffer), size);
}
return {};can we reduce this whole function to:
```
return std::string(base::as_string_view(ColumnBlob(column_index)));
```
(#include "base/strings/string_view_util.h)
CheckCanReadColumn(column_index);
const void* result_buffer = sqlite3_column_blob(ref_->stmt(), column_index);
int size = sqlite3_column_bytes(ref_->stmt(), column_index);
if (result_buffer && size > 0) {
return std::u16string(reinterpret_cast<const char16_t*>(result_buffer),
size / 2);
}
return {};as above, let's avoid duplication. maybe something like:
```
auto byte_span = ColumnBlob(column_index);
base::span u16char_span(reinterpret_cast<const char16_t*>(byte_span.data()),
byte_span.size() / 2);
return std::u16string(base::as_string_view(u16char_span));
```
i wonder if there's an even cleaner way to do the cast.
although typing that out, i see that we run the risk of unaligned reads. so maybe it's better to memcpy the data into the resulting string. perhaps:
```
auto byte_span = ColumnBlob(column_index);
std::u16string result(byte_span.size() / 2, 0u);
std::ranges::copy(byte_span, base::as_writable_byte_span(result).begin());
return result;
```
that has the unfortunate side effect of zero-initializing the output string before putting the data into it, but it should avoid unaligned reads.
CheckCanReadColumn(column_index);
const void* result_buffer = sqlite3_column_blob(ref_->stmt(), column_index);
int size = sqlite3_column_bytes(ref_->stmt(), column_index);
if (result_buffer && size > 0) {
return base::ToVector(base::span(static_cast<const uint8_t*>(result_buffer),
static_cast<size_t>(size)));
}
return {};```
auto byte_span = ColumnBlob(column_index);
return std::vector<uint8_t>(byte_span.begin(), byte_span.end());
```
(and remove the #include of to_vector.h)
Related details
- Evan Stade
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Evan Stade (Gerrit)
Evan Stade added 11 comments![]( "Open in Gerrit")
this value is only needed within the `if`, so:
```
if (auto blob = ...; !blob.empty()) {
...
}
```
Done
let's just inline `s.ColumnBlobAsString(0)` here
Done
results.push_back(std::move(encrypted_note));Evan Stade`s.ColumnBlobAsString(0)` inline
Done
if (entry_ok) {Evan Stadeinline `!service.empty()` here
Done
CHECK_EQ(last_sqlite_result_code_.value(), SqliteResultCode::kRow)this will cause a crash within the call to `value()` if `last_sqlite_result_code_` doesn't hold a value. this might be a bit confusing to diagnose if it happens. wdyt about changing this to something like `.value_or(SqliteResultCode::kOk)` so that the `CHECK_EQ` will fire in all cases?
Using `.value_or(SqliteResultCode::kOk)` wouldn't distinguish between when the value was actually `kOk` and when it was missing, so I guess that a separate `CHECK(has_value())` seems the least confusing/ambiguous to me.
<< " reading from columns must happen after Step() is called and "
"returns true";strings like this take up space in the binary for little benefit. please move this into a comment that immediately precedes the `CHECK_EQ`. it will be trivial to find if this `CHECK` is ever hit.
Done
CHECK_GE(column_index, 0, base::NotFatalUntil::M142);please retain this check. if you don't think it's necessary: why?
oops, restored.
const void* result_buffer = sqlite3_column_blob(ref_->stmt(), column_index);while you're here, please swap the order of these two lines as per https://www.sqlite.org/c3ref/column_blob.html: "you should call sqlite3_column_text(), sqlite3_column_blob(), or sqlite3_column_text16() first to force the result into the desired format, then invoke sqlite3_column_bytes() or sqlite3_column_bytes16() to find the size of the result."
Done
CheckCanReadColumn(column_index);
const void* result_buffer = sqlite3_column_blob(ref_->stmt(), column_index);
int size = sqlite3_column_bytes(ref_->stmt(), column_index);
if (result_buffer && size > 0) {
return std::string(reinterpret_cast<const char*>(result_buffer), size);
}
return {};can we reduce this whole function to:
```
return std::string(base::as_string_view(ColumnBlob(column_index)));
```
(#include "base/strings/string_view_util.h)
Done
CheckCanReadColumn(column_index);
const void* result_buffer = sqlite3_column_blob(ref_->stmt(), column_index);
int size = sqlite3_column_bytes(ref_->stmt(), column_index);
if (result_buffer && size > 0) {
return std::u16string(reinterpret_cast<const char16_t*>(result_buffer),
size / 2);
}
return {};as above, let's avoid duplication. maybe something like:
```
auto byte_span = ColumnBlob(column_index);
base::span u16char_span(reinterpret_cast<const char16_t*>(byte_span.data()),
byte_span.size() / 2);
return std::u16string(base::as_string_view(u16char_span));
```
i wonder if there's an even cleaner way to do the cast.although typing that out, i see that we run the risk of unaligned reads. so maybe it's better to memcpy the data into the resulting string. perhaps:
```
auto byte_span = ColumnBlob(column_index);
std::u16string result(byte_span.size() / 2, 0u);
std::ranges::copy(byte_span, base::as_writable_byte_span(result).begin());
return result;
```
that has the unfortunate side effect of zero-initializing the output string before putting the data into it, but it should avoid unaligned reads.
Done, but this makes me wonder if this one's signature needs to remain as is, in case the blob has an odd number of bytes. That should probably be reported as an error rather than ignored. WDYT?
CheckCanReadColumn(column_index);
const void* result_buffer = sqlite3_column_blob(ref_->stmt(), column_index);
int size = sqlite3_column_bytes(ref_->stmt(), column_index);
if (result_buffer && size > 0) {
return base::ToVector(base::span(static_cast<const uint8_t*>(result_buffer),
static_cast<size_t>(size)));
}
return {};```
auto byte_span = ColumnBlob(column_index);
return std::vector<uint8_t>(byte_span.begin(), byte_span.end());
```
(and remove the #include of to_vector.h)
- Greg Thompson
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Greg Thompson (Gerrit)
Greg Thompson voted and added 5 comments![]( "Open in Gerrit")
Votes added by Greg Thompson
| Code-Review | +1 |
5 comments
CHECK_EQ(last_sqlite_result_code_.value(), SqliteResultCode::kRow)Evan Stadethis will cause a crash within the call to `value()` if `last_sqlite_result_code_` doesn't hold a value. this might be a bit confusing to diagnose if it happens. wdyt about changing this to something like `.value_or(SqliteResultCode::kOk)` so that the `CHECK_EQ` will fire in all cases?
Using `.value_or(SqliteResultCode::kOk)` wouldn't distinguish between when the value was actually `kOk` and when it was missing, so I guess that a separate `CHECK(has_value())` seems the least confusing/ambiguous to me.
Acknowledged
CHECK_EQ(last_sqlite_result_code_.value(), SqliteResultCode::kRow);nit: now that the line above asserts that this holds a value, please switch to `*last_sqlite_result_code_` here.
std::ranges::copy(bytes, base::as_writable_byte_span(result).begin());Please fix this WARNING reported by ClangTidy: check: misc-include-cleaner
no header providing "std::ranges::copy" is directly...
check: misc-include-cleaner
no header providing "std::ranges::copy" is directly included (https://clang.llvm.org/extra/clang-tidy/checks/misc/include-cleaner.html)
(Note: You can add `Skip-Clang-Tidy-Checks: misc-include-cleaner` footer to the CL description to skip the check)
(Lint observed on `android-clang-tidy-rel` and `linux-clang-tidy-rel`)
CheckCanReadColumn(column_index);
const void* result_buffer = sqlite3_column_blob(ref_->stmt(), column_index);
int size = sqlite3_column_bytes(ref_->stmt(), column_index);
if (result_buffer && size > 0) {
return std::u16string(reinterpret_cast<const char16_t*>(result_buffer),
size / 2);
}
return {};Evan Stadeas above, let's avoid duplication. maybe something like:
```
auto byte_span = ColumnBlob(column_index);
base::span u16char_span(reinterpret_cast<const char16_t*>(byte_span.data()),
byte_span.size() / 2);
return std::u16string(base::as_string_view(u16char_span));
```
i wonder if there's an even cleaner way to do the cast.although typing that out, i see that we run the risk of unaligned reads. so maybe it's better to memcpy the data into the resulting string. perhaps:
```
auto byte_span = ColumnBlob(column_index);
std::u16string result(byte_span.size() / 2, 0u);
std::ranges::copy(byte_span, base::as_writable_byte_span(result).begin());
return result;
```
that has the unfortunate side effect of zero-initializing the output string before putting the data into it, but it should avoid unaligned reads.
Done, but this makes me wonder if this one's signature needs to remain as is, in case the blob has an odd number of bytes. That should probably be reported as an error rather than ignored. WDYT?
good point. this should be documented, no? something about returning an empty string if the column data isn't an integral number of code units. maybe also worth stating that the column data is not checked to ensure that it's valid UTF-16 (e.g., it could contain invalid code points or invalid/incomplete surrogate pairs).
Related details
- Evan Stade
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Evan Stade (Gerrit)
Evan Stade added 3 comments![]( "Open in Gerrit")
CHECK_EQ(last_sqlite_result_code_.value(), SqliteResultCode::kRow);Evan Stadenit: now that the line above asserts that this holds a value, please switch to `*last_sqlite_result_code_` here.
Done
std::ranges::copy(bytes, base::as_writable_byte_span(result).begin());Please fix this WARNING reported by ClangTidy: check: misc-include-cleaner
no header providing "std::ranges::copy" is directly...
check: misc-include-cleaner
no header providing "std::ranges::copy" is directly included (https://clang.llvm.org/extra/clang-tidy/checks/misc/include-cleaner.html)
(Note: You can add `Skip-Clang-Tidy-Checks: misc-include-cleaner` footer to the CL description to skip the check)
(Lint observed on `android-clang-tidy-rel` and `linux-clang-tidy-rel`)
Done
CheckCanReadColumn(column_index);
const void* result_buffer = sqlite3_column_blob(ref_->stmt(), column_index);
int size = sqlite3_column_bytes(ref_->stmt(), column_index);
if (result_buffer && size > 0) {
return std::u16string(reinterpret_cast<const char16_t*>(result_buffer),
size / 2);
}
return {};Evan Stadeas above, let's avoid duplication. maybe something like:
```
auto byte_span = ColumnBlob(column_index);
base::span u16char_span(reinterpret_cast<const char16_t*>(byte_span.data()),
byte_span.size() / 2);
return std::u16string(base::as_string_view(u16char_span));
```
i wonder if there's an even cleaner way to do the cast.although typing that out, i see that we run the risk of unaligned reads. so maybe it's better to memcpy the data into the resulting string. perhaps:
```
auto byte_span = ColumnBlob(column_index);
std::u16string result(byte_span.size() / 2, 0u);
std::ranges::copy(byte_span, base::as_writable_byte_span(result).begin());
return result;
```
that has the unfortunate side effect of zero-initializing the output string before putting the data into it, but it should avoid unaligned reads.
Greg ThompsonDone, but this makes me wonder if this one's signature needs to remain as is, in case the blob has an odd number of bytes. That should probably be reported as an error rather than ignored. WDYT?
good point. this should be documented, no? something about returning an empty string if the column data isn't an integral number of code units. maybe also worth stating that the column data is not checked to ensure that it's valid UTF-16 (e.g., it could contain invalid code points or invalid/incomplete surrogate pairs).
I actually meant that we should revert to the original signature. A blob can validly be empty, so we should distinguish between this type of error and the empty blob case. This error could be indicative of a coding error or database corruption.
I did throw on a `[[nodiscard]]` to make sure callers aren't ignoring this error. We could switch to returning `std::optional<u16string>`, but that leads to somewhat more verbose code at callsites.
Related details
- Colin Blundell
- Greg Thompson
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Evan Stade (Gerrit)
Evan Stade added 1 comment![]( "Open in Gerrit")
+Abhishek ptal database_connection*
+Alex ptal aggregation_service_storage_sql.cc
Related details
- Abhishek Shanthkumar
- Alex Moshchuk
- Colin Blundell
- Greg Thompson
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Greg Thompson (Gerrit)
Greg Thompson added 4 comments![]( "Open in Gerrit")
StatusOr<blink::IndexedDBKeyPath> ColumnKeyPath(sql::Statement& statement,`StatusOr` is [banned](https://chromium.googlesource.com/chromium/src/+/HEAD/styleguide/c++/c++-features.md#statusor-banned); use `base::expected` if you want to return a value in the error case, or `std::optional` otherwise.
std::u16string encoded;
if (!statement.ColumnBlobAsString16(column_index, &encoded)) {
return base::unexpected(Status::Corruption("Key path unexpected size"));
}`ASSIGN_OR_RETURN(std::u16string encoded, statement.ColumnBlobAsString16(column_index, &encoded));` with a lambda if needed to adjust the return type.
ah. `optional` is far better, imo. not only is it more idiomatic and consistent with the style guide, but we have helpers to make it easy for callers; see other comments.
EXPECT_TRUE(select.ColumnBlobAsString16(0, &column_value));
EXPECT_EQ(value, column_value);`EXPECT_THAT(select.ColumnBlobAsString16(0, &column_value), ValueIs(value));`
Related details
- Abhishek Shanthkumar
- Alex Moshchuk
- Colin Blundell
- Evan Stade
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Abhishek Shanthkumar (Gerrit)
Abhishek Shanthkumar voted and added 3 comments![]( "Open in Gerrit")
Votes added by Abhishek Shanthkumar
| Code-Review | +1 |
3 comments
content/browser/indexed_db/instance/sqlite/database_connection.[h,cc] LGTM % a couple of suggestions.
SpecificEvent::kDatabaseNameMismatch));I wonder if reusing this event type is the best approach, and if not logging the event in other places is OK. Could we add a new type and use it everywhere?
StatusOr<blink::IndexedDBKeyPath> key_path = ColumnKeyPath(statement, 3);
if (!key_path.has_value()) {
return base::unexpected(Fatal(key_path.error()));
}
index_metadata.key_path = *std::move(key_path);Can use `ASSIGN_OR_RETURN` I believe?
Related details
- Alex Moshchuk
- Colin Blundell
- Evan Stade
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Alex Moshchuk (Gerrit)
Alex Moshchuk voted and added 2 comments![]( "Open in Gerrit")
Votes added by Alex Moshchuk
| Code-Review | +1 |
+Abhishek ptal database_connection*
+Alex ptal aggregation_service_storage_sql.cc
aggregation_service_storage_sql.cc LGTM, didn't look at the rest.
// LINT.ThenChange(//tools/metrics/histograms/metadata/storage/enums.xml:IndexedDbSqliteSpecificEvent)Is this ERROR reported by If This Then That relevant?
Changes in the preceding block may need to be reflected in these files: /tools/m...
Changes in the preceding block may need to be reflected in these files: /tools/metrics/histograms/metadata/storage/enums.xml If this does not apply, add 'NO_IFTTT=some reason...' to your commit message
Related details
- Colin Blundell
- Evan Stade
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Evan Stade (Gerrit)
Evan Stade added 8 comments![]( "Open in Gerrit")
// LINT.ThenChange(//tools/metrics/histograms/metadata/storage/enums.xml:IndexedDbSqliteSpecificEvent)Is this ERROR reported by If This Then That relevant?
Changes in the preceding block may need to be reflected in these files: /tools/m...
Changes in the preceding block may need to be reflected in these files: /tools/metrics/histograms/metadata/storage/enums.xml If this does not apply, add 'NO_IFTTT=some reason...' to your commit message
yes, fixed.
StatusOr<blink::IndexedDBKeyPath> ColumnKeyPath(sql::Statement& statement,`StatusOr` is [banned](https://chromium.googlesource.com/chromium/src/+/HEAD/styleguide/c++/c++-features.md#statusor-banned); use `base::expected` if you want to return a value in the error case, or `std::optional` otherwise.
This isn't `absl::StatusOr`, this actually is syntactic sugar for `base::expected`, specifically `base::expected<T, leveldb::Status>`
std::u16string encoded;
if (!statement.ColumnBlobAsString16(column_index, &encoded)) {
return base::unexpected(Status::Corruption("Key path unexpected size"));
}`ASSIGN_OR_RETURN(std::u16string encoded, statement.ColumnBlobAsString16(column_index, &encoded));` with a lambda if needed to adjust the return type.
ah, didn't realize that those expected macros could also be applied to optional. Thanks, done.
I wonder if reusing this event type is the best approach, and if not logging the event in other places is OK. Could we add a new type and use it everywhere?
Done
StatusOr<blink::IndexedDBKeyPath> key_path = ColumnKeyPath(statement, 3);
if (!key_path.has_value()) {
return base::unexpected(Fatal(key_path.error()));
}
index_metadata.key_path = *std::move(key_path);Can use `ASSIGN_OR_RETURN` I believe?
Done
CheckCanReadColumn(column_index);
const void* result_buffer = sqlite3_column_blob(ref_->stmt(), column_index);
int size = sqlite3_column_bytes(ref_->stmt(), column_index);
if (result_buffer && size > 0) {
return std::u16string(reinterpret_cast<const char16_t*>(result_buffer),
size / 2);
}
return {};Evan Stadeas above, let's avoid duplication. maybe something like:
```
auto byte_span = ColumnBlob(column_index);
base::span u16char_span(reinterpret_cast<const char16_t*>(byte_span.data()),
byte_span.size() / 2);
return std::u16string(base::as_string_view(u16char_span));
```
i wonder if there's an even cleaner way to do the cast.although typing that out, i see that we run the risk of unaligned reads. so maybe it's better to memcpy the data into the resulting string. perhaps:
```
auto byte_span = ColumnBlob(column_index);
std::u16string result(byte_span.size() / 2, 0u);
std::ranges::copy(byte_span, base::as_writable_byte_span(result).begin());
return result;
```
that has the unfortunate side effect of zero-initializing the output string before putting the data into it, but it should avoid unaligned reads.
Greg ThompsonDone, but this makes me wonder if this one's signature needs to remain as is, in case the blob has an odd number of bytes. That should probably be reported as an error rather than ignored. WDYT?
Evan Stadegood point. this should be documented, no? something about returning an empty string if the column data isn't an integral number of code units. maybe also worth stating that the column data is not checked to ensure that it's valid UTF-16 (e.g., it could contain invalid code points or invalid/incomplete surrogate pairs).
Greg ThompsonI actually meant that we should revert to the original signature. A blob can validly be empty, so we should distinguish between this type of error and the empty blob case. This error could be indicative of a coding error or database corruption.
I did throw on a `[[nodiscard]]` to make sure callers aren't ignoring this error. We could switch to returning `std::optional<u16string>`, but that leads to somewhat more verbose code at callsites.
ah. `optional` is far better, imo. not only is it more idiomatic and consistent with the style guide, but we have helpers to make it easy for callers; see other comments.
Done
EXPECT_TRUE(select.ColumnBlobAsString16(0, &column_value));
EXPECT_EQ(value, column_value);Evan Stade`EXPECT_THAT(select.ColumnBlobAsString16(0, &column_value), ValueIs(value));`
updated.
Related details
- Abhishek Shanthkumar
- Alex Moshchuk
- Colin Blundell
- Greg Thompson
- Mingyu Lei
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Alex Moshchuk (Gerrit)
Alex Moshchuk voted Code-Review+1![]( "Open in Gerrit")
- Abhishek Shanthkumar
- Colin Blundell
- Evan Stade
- Greg Thompson
- Mingyu Lei
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |