Skip to content

Commit

Permalink
Add some missing validation for aggregation on collection properties (#…
Browse files Browse the repository at this point in the history 
…5876)

Results accidentally allowed aggregating a collection of collection properties
as long as those collections stored aggregatable types, with nonsensical
results as it aggregated the refs.
  • Loading branch information
@tgoyne
tgoyne authored Sep 20, 2022
1 parent 128c483 commit f5cfe3d
Show file tree
Hide file tree
Showing 6 changed files with 33 additions and 16 deletions.
4 changes: 2 additions & 2 deletions CHANGELOG.md
View file
Original file line number Diff line number Diff line change
Expand Up @@ -8,8 +8,8 @@ r# NEXT RELEASE

### Fixed
* <How do the end-user experience this issue? what was the impact?> ([#????](https://github.com/realm/realm-core/issues/????), since v?.?.?)
* None.
* Results permitted some nonsensical aggregate operations on column types which do not make sense to aggregate, giving garbage results rather than reporting an error ([#5876](https://github.com/realm/realm-core/pull/5876)).

### Breaking changes
* The typed aggregation functions (e.g. `minimum_int`) on `Table`, `TableView`, and `Query` have been removed and replaced with simpler untyped versions which return `Mixed`. This does not effect SDKs which only used them via the Object Store types.

Expand Down
12 changes: 0 additions & 12 deletions src/realm/object-store/c_api/query.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -458,9 +458,6 @@ RLM_API bool realm_results_min(realm_results_t* results, realm_property_key_t co
bool* out_found)
{
return wrap_err([&]() {
// FIXME: This should be part of Results.
results->get_table()->check_column(ColKey(col));

if (auto x = results->min(ColKey(col))) {
if (out_found) {
*out_found = true;
Expand All @@ -485,9 +482,6 @@ RLM_API bool realm_results_max(realm_results_t* results, realm_property_key_t co
bool* out_found)
{
return wrap_err([&]() {
// FIXME: This should be part of Results.
results->get_table()->check_column(ColKey(col));

if (auto x = results->max(ColKey(col))) {
if (out_found) {
*out_found = true;
Expand All @@ -512,9 +506,6 @@ RLM_API bool realm_results_sum(realm_results_t* results, realm_property_key_t co
bool* out_found)
{
return wrap_err([&]() {
// FIXME: This should be part of Results.
results->get_table()->check_column(ColKey(col));

if (out_found) {
*out_found = results->size() != 0;
}
Expand Down Expand Up @@ -543,9 +534,6 @@ RLM_API bool realm_results_average(realm_results_t* results, realm_property_key_
bool* out_found)
{
return wrap_err([&]() {
// FIXME: This should be part of Results.
results->get_table()->check_column(ColKey(col));

if (auto x = results->average(ColKey(col))) {
if (out_found) {
*out_found = true;
Expand Down
2 changes: 0 additions & 2 deletions src/realm/object-store/c_api/schema.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -183,8 +183,6 @@ RLM_API bool realm_get_property(const realm_t* realm, realm_class_key_t class_ke
auto& os = schema_for_table(*realm, TableKey(class_key));
auto col_key = ColKey(key);

// FIXME: We can do better than linear search.

for (auto& prop : os.persisted_properties) {
if (prop.column_key == col_key) {
*out_property_info = to_capi(prop);
Expand Down
11 changes: 11 additions & 0 deletions src/realm/query_conditions_tpl.hpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -98,6 +98,9 @@ class AggregateHelper {
public:
static std::optional<Mixed> sum(const Table& table, const Target& target, ColKey col_key)
{
table.check_column(col_key);
if (col_key.is_collection())
return std::nullopt;
switch (table.get_column_type(col_key)) {
case type_Int:
if (table.is_nullable(col_key)) {
Expand All @@ -121,6 +124,9 @@ class AggregateHelper {

static std::optional<Mixed> avg(const Table& table, const Target& target, ColKey col_key, size_t* value_count)
{
table.check_column(col_key);
if (col_key.is_collection())
return std::nullopt;
switch (table.get_column_type(col_key)) {
case type_Int:
if (table.is_nullable(col_key)) {
Expand Down Expand Up @@ -170,6 +176,8 @@ class AggregateHelper {
template <typename T>
static Mixed sum(const Target& target, ColKey col_key)
{
if (col_key.is_collection())
return std::nullopt;
QueryStateSum<typename util::RemoveOptional<T>::type> st;
target.template aggregate<T>(st, col_key);
return st.result_sum();
Expand All @@ -178,6 +186,9 @@ class AggregateHelper {
template <template <typename> typename QueryState>
static std::optional<Mixed> minmax(const Table& table, const Target& target, ColKey col_key, ObjKey* return_ndx)
{
table.check_column(col_key);
if (col_key.is_collection())
return std::nullopt;
switch (table.get_column_type(col_key)) {
case type_Int:
if (table.is_nullable(col_key)) {
Expand Down
4 changes: 4 additions & 0 deletions src/realm/table_view.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -244,6 +244,10 @@ template <Action action>
std::optional<Mixed> TableView::aggregate(ColKey column_key, size_t* count, ObjKey* return_key) const
{
static_assert(action == act_Sum || action == act_Max || action == act_Min || action == act_Average);
m_table->check_column(column_key);
if (column_key.is_collection()) {
return std::nullopt;
}

switch (column_key.get_type()) {
case col_type_Int:
Expand Down
16 changes: 16 additions & 0 deletions test/object-store/results.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -4239,6 +4239,7 @@ TEMPLATE_TEST_CASE("results: aggregate", "[query][aggregate]", ResultsFromTable,
{"float", PropertyType::Float | PropertyType::Nullable},
{"double", PropertyType::Double | PropertyType::Nullable},
{"date", PropertyType::Date | PropertyType::Nullable},
{"int list", PropertyType::Int | PropertyType::Array},
}},
{"linking_object",
{
Expand All @@ -4252,6 +4253,8 @@ TEMPLATE_TEST_CASE("results: aggregate", "[query][aggregate]", ResultsFromTable,
ColKey col_float = table->get_column_key("float");
ColKey col_double = table->get_column_key("double");
ColKey col_date = table->get_column_key("date");
ColKey col_int_list = table->get_column_key("int list");
ColKey col_invalid(ColKey::Idx{10}, col_type_Int, {}, 0);

SECTION("one row with null values") {
r->begin_transaction();
Expand All @@ -4271,27 +4274,35 @@ TEMPLATE_TEST_CASE("results: aggregate", "[query][aggregate]", ResultsFromTable,
REQUIRE(results.max(col_float)->get_float() == 2.f);
REQUIRE(results.max(col_double)->get_double() == 2.0);
REQUIRE(results.max(col_date)->get_timestamp() == Timestamp(2, 0));
REQUIRE_THROWS_AS(results.max(col_int_list), Results::UnsupportedColumnTypeException);
REQUIRE_THROWS_AS(results.max(col_invalid), LogicError);
}

SECTION("min") {
REQUIRE(results.min(col_int)->get_int() == 0);
REQUIRE(results.min(col_float)->get_float() == 0.f);
REQUIRE(results.min(col_double)->get_double() == 0.0);
REQUIRE(results.min(col_date)->get_timestamp() == Timestamp(0, 0));
REQUIRE_THROWS_AS(results.min(col_int_list), Results::UnsupportedColumnTypeException);
REQUIRE_THROWS_AS(results.min(col_invalid), LogicError);
}

SECTION("average") {
REQUIRE(results.average(col_int) == 1.0);
REQUIRE(results.average(col_float) == 1.0);
REQUIRE(results.average(col_double) == 1.0);
REQUIRE_THROWS_AS(results.average(col_date), Results::UnsupportedColumnTypeException);
REQUIRE_THROWS_AS(results.average(col_int_list), Results::UnsupportedColumnTypeException);
REQUIRE_THROWS_AS(results.average(col_invalid), LogicError);
}

SECTION("sum") {
REQUIRE(results.sum(col_int)->get_int() == 2);
REQUIRE(results.sum(col_float)->get_double() == 2.0);
REQUIRE(results.sum(col_double)->get_double() == 2.0);
REQUIRE_THROWS_AS(results.sum(col_date), Results::UnsupportedColumnTypeException);
REQUIRE_THROWS_AS(results.sum(col_int_list), Results::UnsupportedColumnTypeException);
REQUIRE_THROWS_AS(results.sum(col_invalid), LogicError);
}
}

Expand Down Expand Up @@ -4378,17 +4389,22 @@ TEMPLATE_TEST_CASE("results: backed by nothing", "[results]", ResultsFromInvalid
TestContext ctx(realm);

ColKey invalid_col;
ColKey well_formed_key(ColKey::Idx{0}, col_type_Int, {}, 0);
SECTION("max") {
REQUIRE(!results.max(invalid_col));
REQUIRE(!results.max(well_formed_key));
}
SECTION("min") {
REQUIRE(!results.min(invalid_col));
REQUIRE(!results.min(well_formed_key));
}
SECTION("average") {
REQUIRE(!results.average(invalid_col));
REQUIRE(!results.average(well_formed_key));
}
SECTION("sum") {
REQUIRE(!results.sum(invalid_col));
REQUIRE(!results.sum(well_formed_key));
}
SECTION("first") {
REQUIRE(!results.first());
Expand Down

0 comments on commit f5cfe3d

Please sign in to comment.