support reflective lookups and setting lookups when parent is inserted before child #195
Conversation
|
@dfruddffdc @afawcett thoughts on this? |
|
@jdrbc so is this when the dependency order is not correctly defined? |
|
@afawcett this is for circular or reflective lookups like A->A or A->B->A. It would also allow setting relationships with an incorrect dependency order. |
|
This PR actually looks very similar to another but without the logging and error handling. The other is also enabled by default (the challenge with a config method is ensuring its set at the correct time, uow factories share uow instance between methods for example). What do you think to this one? https://github.com/financialforcedev/fflib-apex-common/pull/80/files?utf8=%E2%9C%93&diff=unified&w=1 pros and cons? |
|
It is similar, but it doesn't handle the case of A->B->A.
I added the config method because I was concerned with maintaining the behavior of UoW for old projects. The current behavior for UoW is that it silently fails to set these relationships; so if an old project started using the updated version of UoW, then UoW might start start setting relationships where it was failing to set these relationships before. Seems very edge case-y to me but I was not sure how strict you guys were about maintaining old behavior! In the project I'm working on resolving out of order/reflective/cyclic relationships is the default behavior of UoW. Also UoW throws an exception if it discovers that it could not set a relationship. If you were to release a breaking change version of UoW then I think this would be the best behavior. |
|
Ah yes @jdrbc i see what you mean its just solving the A>A (reflective) use case, also i do like the reporting aspects of this implementation as well. So i would like to propose that we focus on this PR to lead the charge. One thing that still concerns me a little is that the configuration of this feature is something that can change throughout the life of the UOW. You can create one, do some stuff, then enable this feature, which i think would be bad and potentially confusing? Do you agree? As such i am wondering if a new constructor arg would be clearer? |
|
I think making the behaviour static over the lifetime of a UOW instance is slightly safer than making it dynamic. But I also think exposing this complexity is going to cause confusion whether it be via a constructor or a config method. The least complicated/confusing approach might be to force this behaviour - I think it is unlikely that this change would break any existing projects... what do you think? I've updated the PR so that the UOW takes a constructor param to enable this behaviour. |
|
Thanks @jdrbc. Yeah i agree it adds complexity, however, there is just no way to tell if it will break things and with the library used by so many it could be bad. One thought i am having hear is to embrace the Application.UnitOfWork factory to allow for a single way to configure the Unit Of Work. That way if we enable a new feature like this its a one line change to enable it and all code using the factory get an instance with it enabled. Its still an opt-in need, but its more localized.... We don't have to include this in this PR, maybe another, what do you think? Meanwhile i think we are good here, but could you switch to camel case for your enum values pleaes, e.g. IgnoreOutOfOrder vs IGNORE_OUT_OF_ORDER. Thanks! 👍 |
|
@jdrbc Can you have a look at the latest comments on this PR around further overloading of the UOW constructor args vs a configuration factory / fluent pattern i am proposing. I would say we don't have to couple your PR with this idea as a dependency btw but you would probably be the last to add yet another explicit param to the UOW constructor tbh... we for sure need a more extensible UOW config solution i feel... anyways please join the chat if you like! #191 |
|
@afawcett I updated the casing and made the behavior attribute final. With the behavior unmodifiable over the lifetime of the uow I think this is best configured via constructor arg. A more extensible UOW would be great! The factory example you gave looks easy to understand. If the changes are substantial enough maybe a breaking change update to fflib would be warrented fflib2_ ? Then this relationship resolution behavior could be default and reduce the complexity of the change |
|
@jdrbc this looks great, really good updates per our discusison. @agarciaffdc what do you think? Ready to merge? |
|
@dbtavernerffdc could you also take a look? |
| * AttemptResolveOutOfOrder - Update child to set the relationship (e.g. insert parent, insert child, update child) | ||
| * IgnoreOutOfOrder (default behaviour) - Do not set the relationship (e.g. leave lookup null) | ||
| */ | ||
| public enum UnresolvedRelationshipBehavior { AttemptResolveOutOfOrder, IgnoreOutOfOrder } |
There was a problem hiding this comment.
@jdrbc is it ok if you change these enum values to camel case?
| @@ -574,6 +628,20 @@ public virtual class fflib_SObjectUnitOfWork | |||
| m_relationships.get(sObjectType.getDescribe().getName()).resolve(); | |||
| m_dml.dmlInsert(m_newListByType.get(sObjectType.getDescribe().getName())); | |||
| } | |||
|
|
|||
| // Resolve any unresolved relationships where parent was inserted after child, and so child lookup was not set | |||
There was a problem hiding this comment.
@jdrbc this code looks like it could all be indented by one level.
| UnresolvedRelationshipBehavior unresolvedRelationshipBehavior) | ||
| { | ||
| m_unresolvedRelationshipBehaviour = unresolvedRelationshipBehavior; | ||
|
|
||
| m_sObjectTypes = sObjectTypes.clone(); |
There was a problem hiding this comment.
What the point of this list cloning?
|
I think to prevent external modification of the private property.
…On Tue, Feb 14, 2023, 16:23 Saranchuk Viktor ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In fflib/src/classes/fflib_SObjectUnitOfWork.cls
<#195 (comment)>
:
> +
+ /**
+ * Constructs a new UnitOfWork to support work against the given object list
+ *
+ * @param sObjectTypes A list of objects given in dependency order (least dependent first)
+ * @param dml Modify the database via this class
+ * @param unresolvedRelationshipsBehaviour If AttemptOutOfOrderRelationships and a relationship is registered
+ * where a parent is inserted after a child then will update the child
+ * post-insert to set the relationship. If IgnoreOutOfOrder then relationship
+ * will not be set.
+ */
+ public fflib_SObjectUnitOfWork(List<Schema.SObjectType> sObjectTypes, IDML dml,
+ UnresolvedRelationshipBehavior unresolvedRelationshipBehavior)
+ {
+ m_unresolvedRelationshipBehaviour = unresolvedRelationshipBehavior;
+
m_sObjectTypes = sObjectTypes.clone();
What the point of this list cloning?
—
Reply to this email directly, view it on GitHub
<#195 (review)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/ACMBUARZKRLWJPF4N36467TWXQOWFANCNFSM4FKQHIRQ>
.
You are receiving this because you were mentioned.Message ID:
<apex-enterprise-patterns/fflib-apex-common/pull/195/review/1298627177@
github.com>
|
Reflective lookups are pretty common & unit of work will currently silently fail to set these lookups on insert. I think it should throw an exception in this case, but that'd probably only be appropriate to add in a 'breaking change' version of fflib.
This might be a good middle ground where the caller can choose to spend the cycles/DML if they wish to set the reflective lookups, or in some strange scenarios set the lookups even when the parent is inserted before the children (e.g. circular lookups A->B->A or A->B->C->A or trigger restrictions).
Thanks for the awesome library! I'm a huge fan.
This change is