Skip to content
Permalink
Browse files

Fixed bug: Regular Model list imports (e.g. Tickets) sequences fail i…

…mport because of attribute that is not required and can't be provided.
  • Loading branch information...
thorsteneckel authored and zammad-sync committed Nov 7, 2019
1 parent 022efe7 commit 1287e9968e74ec62e4273ea199c4ca52c1ae80bf
@@ -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
@@ -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
@@ -86,14 +86,14 @@ def push

# push overviews
results = []
assets = AssetsSet.new
index_and_lists.each do |data|

# do not deliver unchanged lists
next if @last_overview[data[:overview][:id]] == [data[:tickets], data[:overview]]

@last_overview[data[:overview][:id]] = [data[:tickets], data[:overview]]

assets = {}
overview = Overview.lookup(id: data[:overview][:id])
next if !overview

@@ -108,7 +108,8 @@ def push

assets = asset_push(ticket, assets)
end
data[:assets] = assets

data[:assets] = assets.to_h

if !@client
result = {
@@ -125,6 +126,8 @@ def push
data: data,
)
end

assets.flush
end
return results if !@client

@@ -32,9 +32,50 @@
.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

1 comment on commit 1287e99

@martini

This comment has been minimized.

Copy link
Collaborator

martini commented on 1287e99 Nov 7, 2019

Unfortunately wrong commit message, correct one:

Performance: Asset data of same object is gathered and send to the client multiple times/for each overview again.

Please sign in to comment.
You can’t perform that action at this time.