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.

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

fix(db) daos do not send duplicate crud event anymore #4095

Merged
merged 1 commit into from Dec 15, 2018

Conversation

Projects
None yet
2 participants
@bungle
Copy link
Member

commented Dec 14, 2018

Summary

This PR fixes a issue on dao:update* that resulted duplicate crud events to be sent.

@bungle bungle requested review from hishamhm and thibaultcha Dec 14, 2018

@@ -418,7 +418,7 @@ local function generate_foreign_key_methods(schema)
return nil, err, err_t
end

if rbw_entity then
if rbw_entity and self:cache_key(rbw_entity) ~= self:cache_key(row) then

This comment has been minimized.

Copy link
@thibaultcha

thibaultcha Dec 15, 2018

Member

The fix in itself is ok, but a more appropriate one exists.

  1. With this change, we fail to send the update event with the old row (the rbw_entity). That is mostly ok in the core since our dao:crud handler does not do anything specific with the non-cache-key rows, but it could be problematic in a custom plugin or future events handler.
  2. When first implementing this DAO, we stayed away from read-before-write patterns, and as such, imlemented the :post_crud_event() method without the old_entity event metadata. Since we now have read-before-write patterns in the new DAO as well, those 2 lines of code (sending two dao:crud events) better be replaced by the inclusion of the old_entity metadata in :post_crud_event().
diff --git a/kong/db/dao/init.lua b/kong/db/dao/init.lua
index 155acc42..800de474 100644
--- a/kong/db/dao/init.lua
+++ b/kong/db/dao/init.lua
@@ -418,11 +418,7 @@ local function generate_foreign_key_methods(schema)
           return nil, err, err_t
         end
 
-        if rbw_entity then
-          self:post_crud_event("update", rbw_entity)
-        end
-
-        self:post_crud_event("update", row)
+        self:post_crud_event("update", row, rbw_entity)
 
         return row
       end
@@ -758,11 +754,7 @@ function DAO:update(primary_key, entity, options)
     return nil, err, err_t
   end
 
-  if rbw_entity then
-    self:post_crud_event("update", rbw_entity)
-  end
-
-  self:post_crud_event("update", row)
+  self:post_crud_event("update", row, rbw_entity)
 
   return row
 end
@@ -932,12 +924,13 @@ function DAO:row_to_entity(row, options)
 end
 
 
-function DAO:post_crud_event(operation, entity)
+function DAO:post_crud_event(operation, entity, old_entity)
   if self.events then
     local _, err = self.events.post_local("dao:crud", operation, {
       operation = operation,
       schema    = self.schema,
       entity    = entity,
+      old_entity = old_entity,
     })
     if err then
       log(ERR, "[db] failed to propagate CRUD operation: ", err)

This will take advantage of this branch from the dao:crud handler and invalidate the old entity on its own.

In short:

  1. One less worker event to be propagated, and two less local events (broadcasted by the dao:crud handler).
  2. Ability for the handler to act on the old row's values.
  3. We save two cache_key computations.

This comment has been minimized.

Copy link
@bungle

bungle Dec 15, 2018

Author Member

@thibaultcha thanks, very good input!

@bungle bungle force-pushed the fix/dao-double-crud-event branch from 2981370 to 55e6f24 Dec 15, 2018

@thibaultcha

This comment has been minimized.

Copy link
Member

commented Dec 15, 2018

Waiting on CI to be green before merging.

@thibaultcha thibaultcha merged commit cd88563 into next Dec 15, 2018

3 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
license/cla All CLA requirements met.

@thibaultcha thibaultcha deleted the fix/dao-double-crud-event branch Dec 15, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.