-
Notifications
You must be signed in to change notification settings - Fork 480
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
Enforce MAX_TABLE_ROW_COUNT, MAX_TABLE_COUNT, MAX_RECORD_LENGTH, MAX_NUM_KVPS and MAX_VALUE_LENGTH #58598
Enforce MAX_TABLE_ROW_COUNT, MAX_TABLE_COUNT, MAX_RECORD_LENGTH, MAX_NUM_KVPS and MAX_VALUE_LENGTH #58598
Changes from all commits
b6eb702
b507abb
bd2b590
6005f2b
672e869
402daff
52a3d7e
d10576f
8305f2d
a01b5e5
a285484
b062751
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -61,7 +61,6 @@ def index | |
########################################################## | ||
|
||
def set_key_value | ||
raise StudentFacingError, "The value is too large. The maximum allowable size is #{DatablockStorageKvp::MAX_VALUE_LENGTH} bytes" if params[:value].length > DatablockStorageKvp::MAX_VALUE_LENGTH | ||
value = JSON.parse params[:value] | ||
DatablockStorageKvp.set_kvp @project_id, params[:key], value | ||
render json: {key: params[:key], value: value} | ||
|
@@ -219,7 +218,6 @@ def get_columns_for_table | |
########################################################## | ||
|
||
def create_record | ||
raise StudentFacingError, "The record is too large. The maximum allowable size is #{DatablockStorageRecord::MAX_RECORD_LENGTH} bytes" if params[:record_json].length > DatablockStorageRecord::MAX_RECORD_LENGTH | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Removing these in favor of validations on the model |
||
record_json = JSON.parse params[:record_json] | ||
raise "record must be a hash" unless record_json.is_a? Hash | ||
|
||
|
@@ -237,8 +235,6 @@ def read_records | |
end | ||
|
||
def update_record | ||
raise StudentFacingError, "The record is too large. The maximum allowable size is #{DatablockStorageRecord::MAX_RECORD_LENGTH} bytes" if params[:record_json].length > DatablockStorageRecord::MAX_RECORD_LENGTH | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Removing these in favor of validations on the model |
||
|
||
table = find_table | ||
record_json = table.update_record params[:record_id], JSON.parse(params[:record_json]) | ||
table.save! | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -64,6 +64,24 @@ def set_and_get_key_value(key, value) | |
set_and_get_key_value('👁️👄👁️', 'value is: 🎉') | ||
end | ||
|
||
test "set_key_value can't create more than MAX_NUM_KVPS kvps" do | ||
# Lower the max table count to 3 so its easy to test | ||
original_max_num_kvps = DatablockStorageKvp::MAX_NUM_KVPS | ||
DatablockStorageKvp.const_set(:MAX_NUM_KVPS, 3) | ||
|
||
# Create 3 kvps so we're right at the limit... | ||
set_and_get_key_value('key1', 'val1') | ||
set_and_get_key_value('key2', 'val2') | ||
set_and_get_key_value('key3', 'val3') | ||
|
||
post _url(:set_key_value), params: {key: 'key4', value: 'val4'.to_json} | ||
|
||
assert_response :bad_request | ||
assert_equal 'MAX_KVPS_EXCEEDED', JSON.parse(@response.body)['type'] | ||
ensure | ||
DatablockStorageKvp.const_set(:MAX_NUM_KVPS, original_max_num_kvps) | ||
end | ||
|
||
test "set_key_value should enforce MAX_VALUE_LENGTH" do | ||
too_many_bees = 'b' * (DatablockStorageKvp::MAX_VALUE_LENGTH + 1) # 1 more 'b' char than max | ||
post _url(:set_key_value), params: { | ||
|
@@ -72,8 +90,6 @@ def set_and_get_key_value(key, value) | |
} | ||
assert_response :bad_request | ||
|
||
skip "FIXME: controller bug, test will fail, because enforcing DatablockStorageTable::MAX_VALUE_LENGTH_EXCEEDED is not yet implemented, see #57002" | ||
# Is MAX_VALUE_LENGTH_EXCEEDED the right error? check the JS | ||
assert_equal 'MAX_VALUE_LENGTH_EXCEEDED', JSON.parse(@response.body)['type'] | ||
end | ||
|
||
|
@@ -136,7 +152,6 @@ def set_and_get_key_value(key, value) | |
assert_response :success | ||
|
||
post _url(:create_record), params: {table_name: 'table4', record_json: {'name' => 'bob'}.to_json} | ||
skip "FIXME: controller bug, test will fail, because enforcing DatablockStorageTable::MAX_TABLE_COUNT is not yet implemented so we get :success when :bad_request is desired, see #57003" | ||
assert_response :bad_request | ||
assert_equal 'MAX_TABLES_EXCEEDED', JSON.parse(@response.body)['type'] | ||
ensure | ||
|
@@ -155,9 +170,7 @@ def set_and_get_key_value(key, value) | |
|
||
post _url(:create_record), params: {table_name: 'mytable', record_json: {'name' => 'bob'}.to_json} | ||
|
||
skip "FIXME: controller bug, test will fail, because enforcing DatablockStorageTable::MAX_TABLE_ROW_COUNT is not yet implemented so we get :success when :bad_request is desired, see #57002" | ||
assert_response :bad_request | ||
# Is MAX_ROWS_EXCEEDED the right error? check the JS | ||
assert_equal 'MAX_ROWS_EXCEEDED', JSON.parse(@response.body)['type'] | ||
ensure | ||
DatablockStorageTable.const_set(:MAX_TABLE_ROW_COUNT, original_max_table_row_count) | ||
|
@@ -172,8 +185,6 @@ def set_and_get_key_value(key, value) | |
post _url(:create_record), params: {table_name: 'mytable', record_json: {'name' => too_many_bees}.to_json} | ||
assert_response :bad_request | ||
|
||
skip "FIXME: controller bug, test will fail, because enforcing DatablockStorageRecord::MAX_RECORD_LENGTH is not yet implemented at the DatablockStorage level, see #57001" | ||
# Is MAX_RECORD_LENGTH_EXCEEDED the right error? check the JS | ||
assert_equal 'MAX_RECORD_LENGTH_EXCEEDED', JSON.parse(@response.body)['type'] | ||
end | ||
|
||
|
@@ -621,26 +632,26 @@ def create_shared_table | |
end | ||
|
||
test "import_csv" do | ||
CSV_DATA = <<~CSV | ||
csv_data = <<~CSV | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Sorry for the noise on these, I guess CAPS means its a class level constant, and ruby was generating warnings, this makes the warnings go away |
||
id,name,age,male | ||
4,alice,7,false | ||
5,bob,8,true | ||
6,charlie,9,true | ||
CSV | ||
|
||
EXPECTED_RECORDS = [ | ||
expected_records = [ | ||
{"id" => 1, "name" => "alice", "age" => 7, "male" => false}, | ||
{"id" => 2, "name" => "bob", "age" => 8, "male" => true}, | ||
{"id" => 3, "name" => "charlie", "age" => 9, "male" => true}, | ||
] | ||
|
||
post _url(:import_csv), params: { | ||
table_name: 'mytable', | ||
table_data_csv: CSV_DATA, | ||
table_data_csv: csv_data, | ||
} | ||
assert_response :success | ||
|
||
assert_equal EXPECTED_RECORDS, read_records | ||
assert_equal expected_records, read_records | ||
end | ||
|
||
test "import_csv overwrites existing data" do | ||
|
@@ -650,31 +661,31 @@ def create_shared_table | |
} | ||
assert_response :success | ||
|
||
CSV_DATA = <<~CSV | ||
csv_data = <<~CSV | ||
id,name | ||
1,alice | ||
2,bob | ||
3,charlie | ||
CSV | ||
|
||
EXPECTED_RECORDS = [ | ||
expected_records = [ | ||
{"id" => 1, "name" => "alice"}, | ||
{"id" => 2, "name" => "bob"}, | ||
{"id" => 3, "name" => "charlie"}, | ||
] | ||
|
||
post _url(:import_csv), params: { | ||
table_name: 'mytable', | ||
table_data_csv: CSV_DATA, | ||
table_data_csv: csv_data, | ||
} | ||
assert_response :success | ||
|
||
# Tim, age 2 record should be gone: | ||
assert_equal EXPECTED_RECORDS, read_records | ||
assert_equal expected_records, read_records | ||
end | ||
|
||
test "export csv" do | ||
CSV_DATA = <<~CSV | ||
csv_data = <<~CSV | ||
id,name,age,male | ||
1,alice,7,false | ||
2,bob,8,true | ||
|
@@ -683,7 +694,7 @@ def create_shared_table | |
|
||
post _url(:import_csv), params: { | ||
table_name: 'mytable', | ||
table_data_csv: CSV_DATA, | ||
table_data_csv: csv_data, | ||
} | ||
assert_response :success | ||
|
||
|
@@ -692,7 +703,7 @@ def create_shared_table | |
} | ||
assert_response :success | ||
|
||
assert_equal CSV_DATA, @response.body | ||
assert_equal csv_data, @response.body | ||
end | ||
|
||
test "project_has_data" do | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removing these in favor of validations on the model