Bug #2893

Prevent symbols and fancy Hash subclasses from going in or out of the database in serialized attributes

Added by Tom Clegg over 6 years ago. Updated over 6 years ago.

Status:
Resolved
Priority:
Normal
Assigned To:
Category:
-
Start date:
05/29/2014
Due date:
% Done:

100%

Estimated time:
(Total: 1.00 h)
Story points:
1.0

Subtasks

Task #2894: Review 2893-no-symbols-in-dbResolvedRadhika Chippada

Associated revisions

Revision 21485541
Added by Tom Clegg over 6 years ago

2893: Merge branch '2893-no-symbols-in-db' closes #2893

Revision 135f306e (diff)
Added by Tom Clegg over 6 years ago

2893: Fix accepting JSON-encoded components_summary in API calls. refs #2893

History

#1 Updated by Tom Clegg over 6 years ago

  • Assigned To set to Tom Clegg

#2 Updated by Radhika Chippada over 6 years ago

Review comments:

1. How about the method name "has_symbols?" instead of "has_any_symbols?"

2. convert_serialized_symbols_to_strings method

This method checks "if attr.object_class == Hash"
Even though we have used only Hash type objects for serialized attributes, can't I have an array defined as seriazed attribute? For ex: "serialize :my_attr, Array"
Should this method hence also support Array objects as well? What if I said "serialize :my_attr, String" or some other db supported object?
Should this method intead omit this check and invoke "if self.class.has_any_symbols? attributes[colname]" for all cases or all supported cases?

3. Please add the below additional tests to ArvadosModelTest -> test "refuse symbol keys in serialized attribute: #{x.inspect}"

--- a/services/api/test/unit/arvados_model_test.rb
+++ b/services/api/test/unit/arvados_model_test.rb
@ -32,6 +32,12 @ class ArvadosModelTest < ActiveSupport::TestCase

[ {:a => 'foo'},
+ {'a' => :foo},
+ {:a => ['foo', 'bar']},
+ {'a' => [:foo, 'bar']},
+ {:a => [:foo, :bar]},
+ {:a => {'foo' => {'bar' => 'baz'}}},
+ {'a' => {:foo => {'bar' => 'baz'}}}, {'a' => {'foo' => {:bar => 'baz'}}}, {'a' => {'foo' => {'bar' => :baz}}},

#3 Updated by Tom Clegg over 6 years ago

  • Target version changed from 2014-05-28 Pipeline Factory to 2014-06-17 Curating and Crunch

#4 Updated by Tom Clegg over 6 years ago

Now at 15cc0bc

  • Renamed to has_symbols?
  • Commented convert_serialized_symbols_to_strings
  • Convert all serialized types in convert_serialized_symbols_to_strings, not just Hash
  • Added test cases as suggested
  • Improved readability of has_symbols?

#5 Updated by Anonymous over 6 years ago

  • Status changed from In Progress to Resolved

Applied in changeset arvados|commit:21485541dec5b6df36aaba7d4c2a1e96ba65dec6.

Also available in: Atom PDF