Is this a good use of monkey patching?

Question

I have a set of data-struct like classes that correspond to the JSON messages being exchanged between services in an overall architecture. By struct like, I mean these classes just have fields -- no member functions -- and are implemented using pydantic since this makes for easy development using fastapi. There is currently a design decision to keep these classes "pure" in the sense that they only define data members (think C-struct). These classes are nested in the sense that some of the classes have members that are (collections of) other classes defined here.

In order to support good design within a service, I believe it is desirable to define member functions for operating on/with these data classes. For example, consider the case below:

class Disk(pydantic.BaseModel):
     x: float
     y: float
     r: float
class Scene(pydantic.BaseModel):
     disks: list[Disk]
     name: str

To me, it is natural for there to be a Disk.hit(self, x:float, y:float) -> bool that returns True if the arguments define a point that is within the radius (r) of the center of the disk. Similarly, a function like Scene.hits(self, x, y) -> int that returns the number of disks that lie under a given point makes sense.

Is this a valid use case for using monkey-patching in order to add these member functions after the fact? Are there risks/drawbacks other than the function-name collision one (which should be manageable by constraining the monkey patching to only occur within one specific package)?

This just seems like a regular object, with extra steps. Do you want these to be plain old data objects with no methods (i.e., a DTO), or do you not? Because right now it's neither a method-less data object, nor is it a simple object with useful methods. — Alexander, Nov 08 '22 at 21:32
Is the package where you define these types the same as the package where you want to add the patch? — Caleth, Nov 09 '22 at 14:05
@Caleth currently yes. However if the fleshed out classes end up being only used within only the one service we could (IMHO should) move the definition of them into the codebase (and thus packaged with) for that service. — Dave, Nov 09 '22 at 14:51

score 5 · Answer 1 · answered Nov 09 '22 at 10:54

No, that is not a good use of monkey-patching.

You are using the monkey-patch possibility to circumvent a design decision that has a negative impact on how you would like to design your code. Trying to circumvent such decisions is never a good idea.

There are two good courses of action:

Challenge the design decision and argue why it would increase the code quality if you can have some methods added to those pure structures
Accept the design decision and write your functions in another way, for example as stand-alone functions that take a Disk or Scene as argument.

Extending a 3rd party package by mix-ins or own extensions isn't uncommon and not necessarily a bad decision. — Doc Brown, Nov 11 '22 at 16:22

Doc Brown · Answer 2 · 2022-11-13T10:38:08.317

Let us assume the data-struct like classes are coming from an external package or 3rd party where you cannot change easily the source.

Now your application or package which uses these classes wants to extend them by a few methods. In a language like C#, we would use extension methods for this, this is a standard use case and perfectly justified. In Python, a similar result can be achieved by "monkey patching" and can be used for essentially the same purpose. Because of the fact your original classes don't contain any former member functions, there is effectively no risk to replace an existing method by something different accidentally.

So the answer is IMHO yes, this can be a standard use case.

However, there is one caveat: if the 3rd party package (or another one) contains some generic code which relies on the classes containing only data members (maybe some persistence mechanics, which does something like iterating over all members of a class), then monkey patching might break that code (which is different from extension methods in C#, which are more like syntactic sugar around static methods). So you should check whether this might be the case in your situation, or if you think the risk is low.

score 0 · Answer 3 · answered Nov 09 '22 at 14:11

There are primitive types that should be wrapped in value classes (like Time based on a long), and there are value classes of several fields, structs, like that Disk). Those classes closely associate with their operations, and hence I also would assume hit to be an inherent method.

But then there are messages, DTOs, parameter and context related classes. They should not have methods, besides in the future for API changes with backward compatible adapter methods.

So I would take the stand that:

---------------------------------------- Primitive / Value Types
class DiskValue:
     x: float
     y: float
     r: float
     hit: ...
---------------------------------------- API
class Disk(pydantic.BaseModel):
     disk: DiskValue
class Scene(pydantic.BaseModel):
     disks: list[Disk]                   or even list[DiskValue]
     name: str
----------------------------------------

These layers keep a useful abstraction with its algebraic set of operations, with a kind of DRY principle (do not repeat yourself).

The problem with this is two parallel sets of classes `DiskValue, SceneValue etc.` and `Disk, Scene etc.` that need to be maintained separately — Dave, Nov 09 '22 at 14:48
Scene not so much, only classes like Point, Date and Time. But yeah. — Joop Eggen, Nov 09 '22 at 15:08

Is this a good use of monkey patching?

3 Answers3