functions: Do not crash when schema is missing #18639
Conversation
Getting token() function first tries to find a schema for underlying table and continues with nullptr if there's no one. Later, when creating token_fct, the schema is passed as is and referenced. If it's null crash happens. It used to throw before 5983e9e (cql3: test_assignment: pass optional schema everywhere) on missing schema, but this commit changed the way schema is looked up, so nullptr is now possible. fixes: scylladb#18637 Signed-off-by: Pavel Emelyanov <xemul@scylladb.com>
xemul
added
the
backport/5.4
🔴 CI State: FAILURE✅ - Build Failed Tests (1/30706):Build Details:
|
|
🟢 CI State: SUCCESS✅ - Build Build Details:
|
|
@scylladb/scylla-maint , please merge |
| @@ -332,6 +332,9 @@ functions::get(data_dictionary::database db, | |||
| if (!receiver_cf.has_value()) { | |||
| throw exceptions::invalid_request_exception("functions::get for token doesn't have a known column family"); | |||
| } | |||
| if (schema == nullptr) { | |||
| throw exceptions::invalid_request_exception(format("functions::get for token cannot find {} table", *receiver_cf)); | |||
| } | |||
There was a problem hiding this comment.
mutation_fragments(tab) should have a pseudo-schema. Oh well.
There was a problem hiding this comment.
It has pseudo-schema, but it's temporary and not find-able via database/data_disctionary, thus this issue
There was a problem hiding this comment.
We could inject it into the schema registry for the duration of the query.
What query reproduces this problem?
|
This PR needed a regression test (a test that crashes before this patch, and passes afterwards). It's easy with the example in #18637. |
@avikivity I have an alternative proposal: let's avoid merging bugfixes without regression tests, especially if it's something that can be very easily amended, like when the original issue has a reproducer already attached to it, so regression test can be created with minimum effort? |
|
Absolutely agree, but if the maintainer did not follow this rule, the next best thing is to ask for a follow up. |
Getting token() function first tries to find a schema for underlying table and continues with nullptr if there's no one. Later, when creating token_fct, the schema is passed as is and referenced. If it's null crash happens.
It used to throw before 5983e9e (cql3: test_assignment: pass optional schema everywhere) on missing schema, but this commit changed the way schema is looked up, so nullptr is now possible.
fixes: #18637