Enhance persister serialization #17361
Conversation
Cleanup Persister serialization interface. Renaming to to_hash/from_hash methods. Deleting YAML serialization for now, since it's way slower and we have problems that it keeps Symbols around.
Move IC serialization to it's own class, this way it's more concise what everything we need to do to serialize InventoryCollection, plus we can safely guard the recursively nested lazy links.
Assert that reference exists before using it
Correct the wrong type in YARD doc
Ladas
force-pushed
the
enhance_persister_serialization
branch
from
461ad18
to
7a02459
Compare
Add basic specs for persister serialization
Ladas
force-pushed
the
enhance_persister_serialization
branch
from
7a02459
to
e0efeab
Compare
| allow(Settings.ems_refresh).to receive(:mock).and_return({}) | ||
| end | ||
|
|
||
| let(:persister) { create_persister } |
There was a problem hiding this comment.
Do you need this? You're setting it to a local var here
There was a problem hiding this comment.
right, I don't use that one, let me delete it
Remove unused :let and typo in spec
|
Checked commits Ladas/manageiq@6971ccb~...b96e4ce with ruby 2.3.3, rubocop 0.52.1, haml-lint 0.20.0, and yamllint 1.10.0 spec/models/manager_refresh/persister/serializing_spec.rb
spec/models/manager_refresh/save_inventory/single_inventory_collection_spec.rb
|
| # @param value [ManagerRefresh::InventoryObject, ManagerRefresh::InventoryObjectLazy] InventoryObject or a lazy link | ||
| # @param depth [Integer] Depth of nesting for nested lazy link | ||
| # @return [Hash] Serialized ManagerRefresh::InventoryObjectLazy | ||
| def lazy_relation_to_hash(value, depth = 0) |
There was a problem hiding this comment.
Is depth used for anything besides checking if there is a lazy ref chain that is "too long"?
There was a problem hiding this comment.
no, it just gives us nice exception, not relying to stack level too deep exception
| elsif value.kind_of?(Array) && (inventory_object_lazy?(value.compact.first) || inventory_object?(value.compact.first)) | ||
| value.compact.map { |x| lazy_relation_to_hash(x, depth) } | ||
| elsif inventory_object?(value) | ||
| lazy_relation_to_hash(value, depth) |
There was a problem hiding this comment.
Should this case be merged with https://github.com/ManageIQ/manageiq/pull/17361/files#diff-7a3a5be928f76ef87ac9cee741b1ee43R117 ?
If lazy_relation_to_hash can handle inventory objects as well as lazy inventory objects should it be renamed to something more general?
There was a problem hiding this comment.
Actually it looks like it is always setting the type to lazy, https://github.com/ManageIQ/manageiq/pull/17361/files#diff-7a3a5be928f76ef87ac9cee741b1ee43R57
Will this work with a regular inventory object?
There was a problem hiding this comment.
right, all links are trasfered to lazy_find, since that is the only one that is serializable
There was a problem hiding this comment.
right, I'll might merge it to 1 branch
| hash_to_lazy_relation(value, available_inventory_collections, depth) | ||
| elsif value.kind_of?(Array) && value.first.kind_of?(Hash) && value.first['type'] == "ManagerRefresh::InventoryObjectLazy" | ||
| # TODO(lsmola) do we need to compact it sooner? What if first element is nil? On the other hand, we want to | ||
| # deprecate Vmthis HABTM assignment because it's not effective |
Enhance persister serialization. Moving it to specific class and making serialization of nested lazy_find work. Adding basic specs to verify lazy_finds still work after deserialization.