Adding work to UOW breaks factory pattern about fflib-apex-common HOT 20 CLOSED

Sorry Jon can you elaborate a bit more here, i'm not sure what your saying is broken?

from fflib-apex-common.

Sorry for the confusion Andrew. fflib_SObjectUnitOfWork has registerWork/registerEmail methods exposed as public. When using fflib_Application.UnitOfWorkFactory, newInstance returns a fflib_ISObjectUnitOfWork. The interface fflib_ISObjectUnitOfWork does not contain registerWork/registerEmail so these are not able to be called without having a direct instance of fflib_SObjectUnitOfWork. Unless not exposing these was intentional, adding these two methods to the interface fflib_ISObjectUnitOfWork would allow consumers of a UOW to register work/email. Hope this helps...thanks!

from fflib-apex-common.

Ah got it! Yes, so we need to add those methods on the interface. That's an easy one! 👍

from fflib-apex-common.

At least I made one thing easy :) Thanks!

from fflib-apex-common.

I actually tried to handle registerWork before looking at this issue, figured out that I needed to add this methods on the interface and wanted to provide a patch.

However it seems more complicated that I had expected.

The registerWork signature is

 public void registerWork(IDoWork work);

The method is expecting an interface for its parameter. This interface is defined in the ffllib_SObjectUnitOfWork class

So you can't simply add

 void registerWork(IDoWork work);

in the fflib_ISObjectUnitOfWork interface because it would say the IDoWork is an invalid type. However I don't really know how to expose the IDoWork interface to fflib_ISObjectUnitOfWork (don't know if this last sentence makes any sense).

from fflib-apex-common.

I think that I have figured out how to deal with that. The drawback is that it takes the IDoWork interface out of the fflib_SObjectUnitOfWork class. But could it be done in another way?
I have opened PR #61

from fflib-apex-common.

@cirotix Thanks for addressing this. I think you have two options for this: 1) In the ISObjectUnitOfWork, add the methods registerWork/Email and the parameter type would be fflib_SObjectUnitOfWork.IDoWork; 2) The way you have it in the PR which is to define the interface seperately and then consume in fflib_ISObjectUnitOfWork and fflib_SObjectUnitOfWork; . I think the approach you have now is the cleanest and it decouples "IDoWork" from the concrete implementation fflib_SObjectUnitOfWork. Just my $0.02 though, either approach would work technically speaking. If you want to minimize introducing potential regressions, then approach #1 would meet that goal as the existing fflib_SObjectUnitOfWork.registerWork method would remain exactly the same. With approach #2, you are technically changing the method signature.

from fflib-apex-common.

HI @jondavis9898

While I initially thought at option 1, I don't think that it is very cashier in the religion of OOP as an interface should not depend on a concrete implementation of itself. I agree on the fact that it should work.

As for the regressions I think solutions are equal as my solution change the fflib_ISObjectUnitOfWork interface. All the concrete implementations while have to be updated.

from fflib-apex-common.

Hi @cirotix -

Agreed completely that better design/OOP approach is #2 as you have in PR. As for regressions, the fflib_SObjectUnitOfWork already contains registerWork/registerEmail methods so if you went with #1, the only change required should be adding the methods to the interface (no changes to concrete class). If this was all brand new code, I would advocate for #2 as, like you said, its a better design. I think #1 is really only an option if eliminating regressions is important as the only thing required in this case should be adding the two methods to the interface since the concrete class already has the methods. Given that the the registerWork is not virtual and cannot be overridden currently, the only risk of regression is that fflib consumers are obtaining direct instances to fflib_SObjectUnitOfWork (instead of fflib_ISObjectUnitOfWork) and calling these methods or if others are implementing their own concrete base class and not using the libraries. I'm not sure how widely used these methods are but the risk there is essentially the only reason to consider option #1 (if someone is creating their own concrete base class, they would have to make a chance with either option #1 or #2).

Thanks again for taking this on!

from fflib-apex-common.

Great discussion! 👍

I really miss Java packages in Apex sometimes! As an original motivation for using an inner interface is simply containment of the UOW "module" classes/interfaces. As I do get from folks now and again that the top level class foot print of the library is a little intimidating.

Aside from that your both correct, it not 100% great practice to define it in a concreate impl class. I do recall a few questions on IDoWork in the past so i suspect thats been implemented by a few folks. Thus on balance i'm going to suggest we let backwards compatibility lead us here. Thus option 2.

Technically adding methods to an interface also break backwards compatibility. Though these interfaces are really used by the mocking framework and thus the proxies are implemented for the library consumer. Its less likely that someone will have implemented the UOW interface as such. The only thing i wonder to avoid this might be to use interface extension, creating a IDoWork2 have it extend IDoWork. Thoughts?

from fflib-apex-common.

I agree with you I am not sure that someone has implemented the UOW interface.
As for the backward compatibility, I am fairly new to the SalesForce world so I don't know what are the best pratices. From where I come (web, mostly Python/Django this days) it is ok to break backward compatibility if its bring a better design.

And for such a low level lib I think that it is up the the developers who use it to make sure their code still work with a new version of the the lib.Have you considered some kind of major / minor versioning to handle those situation. Where minor version provides backward compatibily while major can break it?

As for IDoWork2, I fail to understand how it will solve the problem of updating the interface that beaks the backward compatibility. The fflib_ISObjectUnitOfWork has to change to declare registerWork and registerEmail, isn't it?

from fflib-apex-common.

Sorry, i ment fflib_ISObjectUnitOfWork2 extends fflib_ISObjectUnitOfWork. Would avoid breaking backwards compatibility, as we are not changing the published interface.

from fflib-apex-common.

I think Andrew needs to lead us here on a decision since he would be more familiar with how/where the library is being used. It sounds like we all agree that Option #2 (separating the interface from the concrete implementation) is the better design, the only reason for Option #1 (adding methods to interface referencing inner interface) or #1a (adding fflib_ISObjectUnitOfWork2) would be to support backwards compat.

I think fflib_ISObjectUnitOfWork2 would provide a technical solution that would avoid any backwards compat issues, however given my understanding of the library and the fact that it's unlikely anyone has ever written their own UOW using fflib_ISObjectUnitOfWork, it's probably a solution that tries to solve a problem that likely doesn't exist.

Given Andrew's comment earlier about library file bloat and the concern about backwards compat, my thuoght is that we go with Option #1 and leave the interface definition in the concrete class. It's not an ideal design but it accomplishes the need. Moving forward, possibly the guideline for the library is that all interfaces should live on their own. Maybe even a file called fflib_Interfaces that contains any interfaces required to minimize file bloat.

Thoughts?

from fflib-apex-common.

I do agree with @jondavis9898 about fflib_ISObjectUnitOfWork2. I don't really like versionning in classes/interface name (I was actually shocked when I discovered the Product2 standard object, which I guess is named like this for historical reasons, but I may be wrong).

I like the option to have a class fflib_Interfaces that embed all the interfaces of fflib. But going this path would introduce a huge break in backward compatibilty, as all the code implementing the interfaces of fflib would have to be patched to take care of this new organisation.

Other than be intimidating, is the top level class foot print really an issue? After all we have a pseudo namespace with fflib_ appended before all classes/interfaces.

from fflib-apex-common.

@cirotix For the fflib_Interfaces, I was thinking that would be an option moving forward but not something that the existing interfaces would be refactored into. Let's see what Andrew thinks regarding your question on footprint and just general direction on which way to go with this.

from fflib-apex-common.

I've been speaking to another contributor on one of my other repos and he is using IDoWork quite a bit, so we now know of at least one person who would be impacted by moving the interface. So I'm going to help draw this excellent discussion to a close with the view to getting this PR merge. So @jondavis9898 if you could move the interface back where it was, i'd be happy to approve this PR. Thanks! 👍

from fflib-apex-common.

Thanks for the feedback @afawcette. @cirotix initiated the original PR so I went ahead and created PR #67 that reflects your decision to leave the interface in the concrete class.

@cirotix - Hope you don't mind that I created the new PR. Definitely didn't mean to step on your toes, just wanted to get this in as I need it in some work that I'm doing currently. Truly appreciate you taking this on and all your feedback!

from fflib-apex-common.

Hi @jondavis9898
I don't mind, and I am actually starting my vacation, so it is better like this to have it merged quicker.

@afawcett I understand your point. Actually I am also using IDoWork (based on my fork). I will change my code to reflect the decision and switch back to the upstream.

Have a nice summer!

from fflib-apex-common.

Enjoy your vacation @cirotix, thanks again!

from fflib-apex-common.

Thanks both! 👍

from fflib-apex-common.