Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Enforce MAX_TABLE_ROW_COUNT, MAX_TABLE_COUNT, MAX_RECORD_LENGTH, MAX_NUM_KVPS and MAX_VALUE_LENGTH #58598

Merged
merged 12 commits into from
May 15, 2024
Merged

Enforce MAX_TABLE_ROW_COUNT, MAX_TABLE_COUNT, MAX_RECORD_LENGTH, MAX_NUM_KVPS and MAX_VALUE_LENGTH #58598

merged 12 commits into from
May 15, 2024

Conversation

snickell
Copy link
Contributor

@snickell snickell commented May 14, 2024 •
edited

  • Enforces MAX_TABLE_ROW_COUNT, Fixes: #57002
  • Enforces MAX_TABLE_COUNT, Fixes: #57003
  • Enforces MAX_RECORD_LENGTH, Fixes: #57001
  • Enforces MAX_NUM_KVPS, Fixes #57000
  • Enforces MAX_VALUE_LENGTH, Fixes #56999
  • Stops skipping tests for all of the above now that they're working

@snickell snickell requested a review from cnbrenci May 14, 2024 04:04
@snickell snickell changed the title Enforce MAX_TABLE_ROW_COUNT and MAX_TABLE_COUNT Enforce MAX_TABLE_ROW_COUNT, MAX_TABLE_COUNT, MAX_RECORD_LENGTH May 14, 2024
@snickell snickell changed the title Enforce MAX_TABLE_ROW_COUNT, MAX_TABLE_COUNT, MAX_RECORD_LENGTH Enforce MAX_TABLE_ROW_COUNT, MAX_TABLE_COUNT, MAX_RECORD_LENGTH, MAX_NUM_KVPS and May 14, 2024
@snickell snickell changed the title Enforce MAX_TABLE_ROW_COUNT, MAX_TABLE_COUNT, MAX_RECORD_LENGTH, MAX_NUM_KVPS and Enforce MAX_TABLE_ROW_COUNT, MAX_TABLE_COUNT, MAX_RECORD_LENGTH, MAX_NUM_KVPS and MAX_VALUE_LENGTH May 14, 2024
@snickell snickell mentioned this pull request May 14, 2024
Merged
snickell
snickell commented May 14, 2024
View reviewed changes
dashboard/app/controllers/datablock_storage_controller.rb
@@ -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
Copy link
Contributor Author

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

snickell
snickell commented May 14, 2024
View reviewed changes
dashboard/app/controllers/datablock_storage_controller.rb
@@ -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
Copy link
Contributor Author

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

snickell
snickell commented May 14, 2024
View reviewed changes
dashboard/app/controllers/datablock_storage_controller.rb
@@ -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
Copy link
Contributor Author

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

snickell
snickell commented May 14, 2024
View reviewed changes
dashboard/test/controllers/datablock_storage_controller_test.rb
@@ -621,26 +632,26 @@ def create_shared_table
end

test "import_csv" do
CSV_DATA = <<~CSV
csv_data = <<~CSV
Copy link
Contributor Author

Choose a reason for hiding this comment

The 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

cnbrenci
cnbrenci reviewed May 14, 2024
View reviewed changes
dashboard/app/models/datablock_storage_kvp.rb Outdated
@@ -17,17 +17,16 @@ class DatablockStorageKvp < ApplicationRecord

self.primary_keys = :project_id, :key

validate :max_kvp_count, on: :create
validate :max_value_length, on: [:create, :update]

StudentFacingError = DatablockStorageTable::StudentFacingError

# TODO: #56999, implement enforcement of MAX_VALUE_LENGTH, we already have
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remove comment!

cnbrenci
cnbrenci reviewed May 14, 2024
View reviewed changes
dashboard/app/models/datablock_storage_record.rb Outdated

private def max_record_length
if record_json.to_json.bytesize > MAX_RECORD_LENGTH
raise StudentFacingError.new(:MAX_RECORD_LENGTH_EXCEEDED), "JSON data cannot exceed #{MAX_RECORD_LENGTH} bytes"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will a student understand 'JSON data' in this context? Might need a message that aligns more with ui language? 'Table record size' or something?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

or take the wording we landed on before: "The record is too large. The maximum allowable size is #{DatablockStorageRecord::MAX_RECORD_LENGTH} bytes"

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ohhhh yeah, that was a good error message wasn't it 🤣

cnbrenci
cnbrenci approved these changes May 14, 2024
View reviewed changes
Copy link
Contributor

@cnbrenci cnbrenci left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

WOOOOOOOOOOOOOOOOOOO so many issues fixed!!!!!!!!!!!!!!!!!!!!!!!!!!!!! amazing!!!!!!!!!!!

Couple small things and otherwise LGTM!

@snickell snickell merged commit 1488421 into staging May 15, 2024
2 checks passed
@snickell snickell deleted the datablock/enforce branch May 15, 2024 09:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cnbrenci cnbrenci cnbrenci approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
2 participants
@snickell @cnbrenci