diff --git a/lib/assets_set.rb b/lib/assets_set.rb new file mode 100644 index 000000000..11871e22c --- /dev/null +++ b/lib/assets_set.rb @@ -0,0 +1,61 @@ +require_dependency 'assets_set/proxy' + +# The name AssetsSet combines the Assets term in Zammad with the Set class from the Ruby StdLib. +# Zammad Assets serve the purpose to limit requests to the REST API. For a requested object all +# related and relevant objects are collected recursively and then send to the client in one go. +# A Ruby Set implements a collection of unordered values with no duplicates. +# +# This class implements a collection of Zammad Assets with no duplicates. +# This is done by having two structures: +# +# 1st: The actual Assets Hash (e.g. `assets[model_name][object_id] = object_attributes`) +# 2nd: A lookup table for keeping track which elements were added to the actual Assets Hash +# +# The actual Assets Hash should be flushed after sending it to the client. This will keep the +# lookup table untouched. The next time a object that was already send to the client +# should be added to the actual Assets Hash the lookup table will recognize the object +# and will prevent the addition to the actual Assets Hash. +# This way Assets will be send only once to the client for the lifetime of a AssetsSet instance. +class AssetsSet < SimpleDelegator + + # This method overwrites the SimpleDelegator initializer + # to be able to have the actual Assets Hash as an optional argument. + def initialize(assets = {}) + super(assets) + end + + # This method initializes the the global lookup table. + # Each (accessed) Model gets it's own sub structure in it. + def lookup_table + @lookup_table ||= {} + end + + # This method flushes the actual Assets Hash. + def flush + __setobj__({}) + end + + # This method intercepts `assets[model_name]` calls by registering a AssetsSet::Proxy. + # Instead of creating an entry in the actual Assets Hash a AssetsSet::Proxy. + # See AssetsSet::Proxy for further information. + # Existing proxies will be reused. + def [](model) + __getobj__[model] ||= proxy(model) + end + + # This method is used to convert the AssetsSet into a regular Hash which then can be send to the client. + # It cleans up empty Model sub structures in the internal structure. + def to_h + super.delete_if { |_model, assets| assets.blank? }.transform_values!(&:to_h) + end + + private + + def proxy(model) + lookup_table[model] ||= {} + + ::AssetsSet::Proxy.new.tap do |proxy| + proxy.lookup_table = lookup_table[model] + end + end +end diff --git a/lib/assets_set/proxy.rb b/lib/assets_set/proxy.rb new file mode 100644 index 000000000..4e0a64293 --- /dev/null +++ b/lib/assets_set/proxy.rb @@ -0,0 +1,50 @@ +# This class defines a Proxy for accessing objects inside of a AssetsSet Model sub structure. +# +# The Zammad Assets logic works by having an Assets Hash that contains a sub structure for +# each model that is relevant. Before an object gets added to the Model sub structure the +# Model sub structure is checked for the presence of the object by its id. If the object is +# already present it will be skipped. However, if the model is not yet present in the matching +# Model sub structure the Zammad Assets will be collected and added. +# +# We make use of this lookup / add if not present functionality by intercepting calls to the +# actual Assets Hash. +# +# If an object is not yet present in the Model sub structure and should be added +# it will added to the lookup table of the AssetsSet first. After that the object will +# be stored to the actual Assets Hash. +# +# The next time a lookup for an object in the Model sub structure is performed it will find the +# actual object data. However, if the actual Assets Hash is send to the client and the AssetsSet +# is flushed the lookup table is still present. The next time a lookup for an object in the +# Model sub is performed it will NOT find the actual object data. In this case a fall back +# to the lookup table will be performed which will will just return `true` to satisfy the +# "is present" condition +class AssetsSet < SimpleDelegator + class Proxy < SimpleDelegator + + attr_accessor :lookup_table + + # This method overwrites the SimpleDelegator initializer + # to be able to have the actual Assets Hash as an optional argument. + def initialize(assets = {}) + super(assets) + end + + # This method intercepts `assets[model_name][object_id]` calls and return the actual objects data. + # If the object it not present the lookup table of the AssetsSet will be queried. + # If the object was present before a previously performed `flush` it will return true and + # satisfy the "is present" condition in the `assets` of the given model. + # If the object is not and never was present the `assets` logic will be performed as normal. + def [](id) + __getobj__[id] || lookup_table[id] + end + + # This method intercepts `assets[model_name][object_id] = object_attributes` calls. + # It stores an entry in the lookup the of the AssetsSet and then performs the regular call + # to store the data in the actual Assets Hash Model sub structure. + def []=(id, _value) + lookup_table[id] = true + super + end + end +end diff --git a/lib/sessions/backend/ticket_overview_list.rb b/lib/sessions/backend/ticket_overview_list.rb index 7a5133e2e..c54983e82 100644 --- a/lib/sessions/backend/ticket_overview_list.rb +++ b/lib/sessions/backend/ticket_overview_list.rb @@ -86,6 +86,7 @@ class Sessions::Backend::TicketOverviewList < Sessions::Backend::Base # push overviews results = [] + assets = AssetsSet.new index_and_lists.each do |data| # do not deliver unchanged lists @@ -93,7 +94,6 @@ class Sessions::Backend::TicketOverviewList < Sessions::Backend::Base @last_overview[data[:overview][:id]] = [data[:tickets], data[:overview]] - assets = {} overview = Overview.lookup(id: data[:overview][:id]) next if !overview @@ -108,7 +108,8 @@ class Sessions::Backend::TicketOverviewList < Sessions::Backend::Base assets = asset_push(ticket, assets) end - data[:assets] = assets + + data[:assets] = assets.to_h if !@client result = { @@ -125,6 +126,8 @@ class Sessions::Backend::TicketOverviewList < Sessions::Backend::Base data: data, ) end + + assets.flush end return results if !@client diff --git a/spec/lib/sessions/backend/ticket_overview_list_spec.rb b/spec/lib/sessions/backend/ticket_overview_list_spec.rb index 53b53ef5e..ab12394e8 100644 --- a/spec/lib/sessions/backend/ticket_overview_list_spec.rb +++ b/spec/lib/sessions/backend/ticket_overview_list_spec.rb @@ -32,9 +32,50 @@ RSpec.describe Sessions::Backend::TicketOverviewList do .to match_array(Ticket::Overviews.all(current_user: admin).map(&:name)) end + it 'is optimized to not send duplicate asset entries over all events' do + collection_assets = collection.push.map { |hash| hash[:data][:assets] } + + # match all event assets against the assets of the other events + # and make sure that each asset entry is unique over all events assets + unique_asssets = collection_assets.each_with_index.all? do |lookup_assets, lookup_index| + + collection_assets.each_with_index.none? do |comparison_assets, comparison_index| + + # skip assets comparison for same event + next if comparison_index == lookup_index + + # check that none of the lookup_assets assets is present + # in the comparison_assets + lookup_assets.keys.any? do |model| + # skip Models that are only present in our lookup_assets entry + next if !comparison_assets.key?(model) + + # check if there are no intersect Model record IDs + # aka IDs present in both hashes + intersection_ids = lookup_assets[model].keys & comparison_assets[model].keys + intersection_ids.present? + end + end + end + + expect(unique_asssets).to be(true) + end + it 'includes FE assets for all overviews and tickets not pushed in the last two hours' do - expect(collection.push.map { |hash| hash[:data][:assets] }) - .to match_array(Ticket::Overviews.all(current_user: admin).map { |overview| overview.assets({}) }) + + # ATTENTION: we can't compare the arrays of assets directly + # because the Ticket::Overviews backend contain an optimization logic that sends an asset entry only once + # while the Sessions::Backend::* classes results contain all assets for each entry. + # Therefore we merge all assets for each of the both arrays to have two big Hashes that contains all assets. + # See previous example for the matching spec. + + collection_assets = collection.push.map { |hash| hash[:data][:assets] } + collection_assets_merged = collection_assets.each_with_object({}) { |assets, result| result.deep_merge!(assets) } + + overviews_all_assets = Ticket::Overviews.all(current_user: admin).map { |overview| overview.assets({}) } + overviews_all_assets_merged = overviews_all_assets.each_with_object({}) { |assets, result| result.deep_merge!(assets) } + + expect(collection_assets_merged).to eq(overviews_all_assets_merged) end context 'when called twice, with no changes to Ticket and Overview tables' do