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

Make UIDReferenceField to not keep back-references by default #2219

Merged
merged 13 commits into from
Jan 4, 2023
Merged
1 change: 1 addition & 0 deletions CHANGES.rst
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ Changelog
2.4.0 (unreleased)
------------------

- #2219 Make `UIDReferenceField` to not keep back-references by default
- #2209 Migrate AnalysisRequest's ReferenceField fields to UIDReferenceField
- #2218 Improve performance of legacy AT `UIDReferenceField`'s setter
- #2214 Remove `DefaultContainerType` field (stale) from AnalysisRequest
Expand Down
19 changes: 15 additions & 4 deletions src/bika/lims/browser/fields/uidreferencefield.py
Original file line number Diff line number Diff line change
Expand Up @@ -39,9 +39,8 @@ class ReferenceException(Exception):

class UIDReferenceField(StringField):
"""A field that stores References as UID values. This acts as a drop-in
replacement for Archetypes' ReferenceField. A relationship is required
but if one is not provided, it will be composed from a concatenation
of `portal_type` + `fieldname`.
replacement for Archetypes' ReferenceField. If no relationship is provided,
the field won't keep backreferences in referenced objects
"""
_properties = Field._properties.copy()
_properties.update({
Expand All @@ -55,6 +54,17 @@ class UIDReferenceField(StringField):

security = ClassSecurityInfo()

@property
def keep_backreferences(self):
"""Returns whether this field must keep back references. Returns True
if the value for property relationship is None
"""
if not isinstance(self.relationship, six.string_types):
return False
if not self.relationship:
return False
return True
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't understand this logic and the docstring.

How can it return True if the value is None when the first condition checks for strings only and the second condition for falsy values?

IMO the check should be smth. like this:

@property
def keep_backreferences(self):
    relationship = getattr(self, "relationship", None)
    if relationship and isinstance(relationship, six.string_types):
        return True
    return False

But what should be now actually the desired behavior? As far as I understood it should not be linked unless explicitly set to a relationship key...

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed with 4149581 . Sometimes I overthink

Copy link
Contributor

Choose a reason for hiding this comment

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

Okay, this means per default backrefs are not set. Is this currently only the case for client references in analysisrequest or for other references as well?
Would it make sense to purge the old references from these objects? I think about clients with thousands of samples linked as backreferences.

Copy link
Member Author

@xispa xispa Jan 2, 2023

Choose a reason for hiding this comment

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

Okay, this means per default backrefs are not set. Is this currently only the case for client references in analysisrequest or for other references as well?

This applies for all references, regardless of the source/target types. For this PR, I've searched for get_backreferences everywhere (add-ons included) to find the cases for which we use backreferences, but without the relationship being set actually. I then assigned the relationships manually to ensure that everything keeps working.

Would it make sense to purge the old references from these objects? I think about clients with thousands of samples linked as backreferences.

Probably. I would expect a reduction of the size of the serialized objects. Is ok if I open another PR for this purge? Will update the PR in accordance and let you know when ready again


def get_relationship_key(self, context):
"""Return the configured relationship key or generate a new one
"""
Expand Down Expand Up @@ -231,7 +241,8 @@ def set(self, context, value, **kwargs):
uids = filter(api.is_uid, uids)

# Back-reference current object to referenced objects
self._set_backreferences(context, uids, **kwargs)
if self.keep_backreferences:
self._set_backreferences(context, uids, **kwargs)

# Store the referenced objects as uids
if not self.multiValued:
Expand Down
8 changes: 5 additions & 3 deletions src/bika/lims/content/abstractanalysis.py
Original file line number Diff line number Diff line change
Expand Up @@ -66,7 +66,8 @@
Attachment = UIDReferenceField(
'Attachment',
multiValued=1,
allowed_types=('Attachment',)
allowed_types=('Attachment',),
relationship='AnalysisAttachment'
)

# The final result of the analysis is stored here. The field contains a
Expand All @@ -88,7 +89,8 @@

# Returns the retracted analysis this analysis is a retest of
RetestOf = UIDReferenceField(
'RetestOf'
'RetestOf',
relationship="AnalysisRetestOf",
)

# If the result is outside of the detection limits of the method or instrument,
Expand Down Expand Up @@ -1127,7 +1129,7 @@ def getRetestOfUID(self):
def getRawRetest(self):
"""Returns the UID of the retest that comes from this analysis, if any
"""
relationship = "{}RetestOf".format(self.portal_type)
relationship = self.getField("RetestOf").relationship
uids = get_backreferences(self, relationship)
if not uids:
return None
Expand Down
7 changes: 6 additions & 1 deletion src/bika/lims/content/analysisrequest.py
Original file line number Diff line number Diff line change
Expand Up @@ -230,7 +230,12 @@
'Client',
required=1,
allowed_types=('Client',),
relationship='AnalysisRequestClient',
# Do not back-reference. This field is only used for the rendering of
# the reference widget in Add Form. An AnalysisRequest is always
# created inside a Client object. Therefore, there is no need to keep
# back references. Since the Client object is not updated, this should
# also reduce the chance of transaction conflicts as well
relationship=None,
mode="rw",
read_permission=View,
write_permission=FieldEditClient,
Expand Down
1 change: 1 addition & 0 deletions src/bika/lims/content/analysisservice.py
Original file line number Diff line number Diff line change
Expand Up @@ -98,6 +98,7 @@
"Calculation",
schemata="Method",
required=0,
relationship="AnalysisServiceCalculation",
vocabulary="_default_calculation_vocabulary",
allowed_types=("Calculation", ),
accessor="getRawCalculation",
Expand Down
1 change: 1 addition & 0 deletions src/bika/lims/content/calculation.py
Original file line number Diff line number Diff line change
Expand Up @@ -71,6 +71,7 @@
'DependentServices',
required=1,
multiValued=1,
relationship="CalculationDependentServices",
allowed_types=('AnalysisService',),
widget=ReferenceWidget(
checkbox_bound=0,
Expand Down
2 changes: 1 addition & 1 deletion src/senaite/core/profiles/default/metadata.xml
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
<?xml version="1.0"?>
<metadata>
<version>2408</version>
<version>2409</version>
<dependencies>
<dependency>profile-Products.ATContentTypes:base</dependency>
<dependency>profile-Products.CMFEditions:CMFEditions</dependency>
Expand Down
35 changes: 35 additions & 0 deletions src/senaite/core/upgrade/v02_04_000.py
Original file line number Diff line number Diff line change
Expand Up @@ -323,3 +323,38 @@ def migrate_reference_fields(obj, field_names):

# Remove this relationship from reference catalog
ref_tool.deleteReferences(obj, relationship=ref_id)


def rename_retestof_relationship(tool):
"""Renames the relationship for field RetestOf from the format
'<portal-type>RetestOf' to 'AnalysisRetestOf'. This field is inherited by
different analysis-like types and since we now assume that if no
relationship is explicitly set, UIDReferenceField does not keep
back-references, we need to update the relationship for those objects that
are not from 'Analysis' portal_type
"""
logger.info("Rename RetestOf relationship ...")
uc = api.get_tool("uid_catalog")
portal_types = ['DuplicateAnalysis', 'ReferenceAnalysis', 'RejectAnalysis']
brains = uc(portal_type=portal_types)
total = len(brains)
for num, brain in enumerate(brains):
if num and num % 100 == 0:
logger.info("Rename RetestOf relationship {}/{}"
.format(num, total))

if num and num % 1000 == 0:
transaction.savepoint()

# find out if the current analysis is a retest
obj = api.get_object(brain)
field = obj.getField("RetestOf")
retest_of = field.get(obj)
if retest_of:
# re-link referenced object
field.link_reference(retest_of, obj)
Copy link
Contributor

Choose a reason for hiding this comment

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

What happens with the existing references in the annotations?

Copy link
Member Author

Choose a reason for hiding this comment

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

Existing references will be kept:

backrefs[key].append(target_uid)

Copy link
Contributor

Choose a reason for hiding this comment

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

And the references from the old key? Maybe we can remove it before it get's orphane.

Copy link
Member Author

@xispa xispa Jan 2, 2023

Choose a reason for hiding this comment

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

We can remove it yes. Would you mind if I include this removal of orphan references in the PR for the removal of no-longer needed backreferences discussed above, #2219 (comment) ? Will update the PR and let you know when ready again


# Flush the object from memory
obj._p_deactivate()

logger.info("Rename RetestOf relationship [DONE]")
10 changes: 10 additions & 0 deletions src/senaite/core/upgrade/v02_04_000.zcml
Original file line number Diff line number Diff line change
Expand Up @@ -83,4 +83,14 @@
handler="senaite.core.upgrade.v02_04_000.migrate_analysisrequest_referencefields"
profile="senaite.core:default"/>

<!-- Rename RetestOf relationship
https://github.com/senaite/senaite.core/pull/2219 -->
<genericsetup:upgradeStep
title="SENAITE CORE 2.4.0: Rename RetestOf relationship"
description="Rename RetestOf relationship"
source="2408"
destination="2409"
handler="senaite.core.upgrade.v02_04_000.rename_retestof_relationship"
profile="senaite.core:default"/>

</configure>