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 object creation events #549

Merged
merged 8 commits into from Jun 26, 2018
Merged

Fix object creation events #549

merged 8 commits into from Jun 26, 2018

Conversation

buchi
Copy link
Member

@buchi buchi commented Jun 23, 2018

This makes object creation in plone.restapi behave more like TTW object creation.

New objects are now created in the following way:

  1. Create an empty object
  2. Deserialize it
  3. Fire object created event
  4. Add object to container
  5. Fire object added and container modified events

Before that change new objects were created in the this way:

  1. Create an empty object
  2. Fire object created event
  3. Add object to container
  4. Fire object added and container modified events
  5. Deserialize object
  6. Fire modified event

@coveralls
Copy link

coveralls commented Jun 23, 2018

Coverage Status

Coverage decreased (-0.9%) to 96.523% when pulling b612cb3 on fix-object-creation-events into 2c1f9f6 on master.

Copy link
Sponsor Member

@davisagli davisagli left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice job pulling out the right pieces from CMFCore to make this happen. It mostly looks good, but I made one small suggestion.

id_ = obj.getId()
# Archetypes objects are already created in a container thus we just fire
# the notification events.
if container == aq_parent(obj):
Copy link
Sponsor Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Comparing acquisition-wrapped objects with == can be problematic (two different wrappers of the same object are not equal). Probably best to do if aq_base(container) is aq_base(aq_parent(obj))

And maybe we should raise an exception if obj already has a different parent?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, I've amended that.

Not sure, how a different parent would be possible at all? Have you something in mind how this could happen?

@buchi buchi force-pushed the fix-object-creation-events branch from 4f509a1 to c68cc2e Compare June 24, 2018 08:26
@tisto
Copy link
Sponsor Member

tisto commented Jun 24, 2018

@buchi this is at least a possible breaking change, correct?

@buchi buchi force-pushed the fix-object-creation-events branch from c68cc2e to 1a2dadf Compare June 24, 2018 13:34
@buchi
Copy link
Member Author

buchi commented Jun 24, 2018

@tisto It shouldn't break anything unless you're heavily relying on plone.restapi internals, e.g. customizing the endpoint for adding objects.

@tisto tisto added this to the 3.0.0 milestone Jun 24, 2018
@buchi buchi force-pushed the fix-object-creation-events branch from 1a2dadf to b612cb3 Compare June 25, 2018 14:54
@buchi
Copy link
Member Author

buchi commented Jun 25, 2018

@tisto Turns out this can break custom content deserializers. I've added a note to the changelog.

@tisto
Copy link
Sponsor Member

tisto commented Jun 25, 2018

@buchi thank you! Would you mind adding that section to our upgrade guide as well?

@tisto tisto dismissed davisagli’s stale review June 25, 2018 16:52

Issue has been solved.

@tisto tisto self-requested a review June 25, 2018 16:52
else:
rval = container._setObject(id_, obj)
new_id = isinstance(rval, basestring) and rval or id_
return container._getOb(new_id)
Copy link
Sponsor Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

_setObject triggers ObjectAddedEvent which can end up triggering a content rule to move the item to a different container. In this case we might need the same workaround that we use in plone.dexterity.utils.addContentToContainer to work around this edge case:

try:
    return container._getOb(newName)
except AttributeError:
    uuid = IUUID(object)
    return uuidToObject(uuid)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've added that.

Setting `create=True` indicates the creation of a new object and prevents
firing modified events.
Change object creation and deserialization order:
1. Create an empty object
2. Deserialize it
3. Fire object created event
4. Add object to container
5. Fire object added and container modified events
Fixes vocabularies depending on tools in deserialization.
@buchi buchi force-pushed the fix-object-creation-events branch from b612cb3 to ce6e304 Compare June 26, 2018 10:20
@buchi buchi force-pushed the fix-object-creation-events branch from ce6e304 to f112d1c Compare June 26, 2018 11:24
@buchi
Copy link
Member Author

buchi commented Jun 26, 2018

@tisto added small section in the upgrade guide.

@tisto
Copy link
Sponsor Member

tisto commented Jun 26, 2018

@buchi awesome. Thank you!
@davisagli would you mind having another look and approving the merge?

@tisto tisto merged commit d43ef89 into master Jun 26, 2018
@tisto tisto deleted the fix-object-creation-events branch June 26, 2018 13:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants