I agree with everything you said. The problem occurs when a worker receives a task to perform a publish, but it doesn't have the plugin installed. As a result the call to publisher.cast() returns the master model. The publish task then tries to call a method on the master model that does not exist. The user then sees an attribute not found exception in the logs. It would be better to raise a more useful exception in the cast() method itself when it is not able to cast to anything.
One suggestion was to add an optional call_counter keyword argument to cast(self, call_count=0). The argument would only be passed in to cast(call_count=call_count+1) when it is being recursively called. Then we could check if line 113 is reached with call_count==0 and raise an exception saying that the master model could not be cast to a detail model. Thoughts? On Fri, May 26, 2017 at 1:23 PM, Michael Hrivnak <[email protected]> wrote: > Interesting question. It looks like in this implementation, even if you > call cast() on a master model, the method itself will kind-of-recursively > call cast() on detail models until it gets to the most detailed one, which > will return itself. So every time cast() is called, eventually the most > detailed model is expected to have its cast() method called and must return > itself. > > We could add a special case where the last one raises an exception, and > the next-to-last one catches it, but I'm not sure that extra complication > would be worth it. We'd be making the most common case "exceptional". > > Having the call be idempotent is also potentially a perk, depending on how > you look at it. Based on all that, plus the doc block confirming that the > behavior is intentional, I don't see a problem with the current behavior. > > On Fri, May 26, 2017 at 11:41 AM, Dennis Kliban <[email protected]> > wrote: > >> Looking at the cast() method[0] it looks like it's possible to call >> cast() on a detail model. I would like to figure out when we expect to call >> cast() on a detail model. Without fully knowing the motivation for this >> implementation, I am inclined to raise an exception when the code reaches >> line 113. The exception would inform the developer that calling cast() is >> only appropriate on a master model. What are your thoughts? >> >> >> [0] https://github.com/pulp/pulp/blob/3.0-dev/platform/pulp/app/ >> models/base.py#L113 >> >> >> -Dennis >> >> _______________________________________________ >> Pulp-dev mailing list >> [email protected] >> https://www.redhat.com/mailman/listinfo/pulp-dev >> >> > > > -- > > Michael Hrivnak > > Principal Software Engineer, RHCE > > Red Hat >
_______________________________________________ Pulp-dev mailing list [email protected] https://www.redhat.com/mailman/listinfo/pulp-dev
