Track failed condition in TransitionNotAllowed

[knaperek]

Description

Failed condition tracking in TransitionNotAllowed for better diagnosis and fine-grained error handling.
Fully backwards compatible, this only adds advisory context to the exception.

Type of Change

• Bug fix (non-breaking change that fixes an issue)
• New feature (non-breaking change that adds functionality)
• Breaking change (fix or feature that would cause existing functionality to change)
• Documentation update
• Other (no functional changes)

Checklist

• I have read the CONTRIBUTING.md guide
• Tests added/updated for the changes
• All tests pass locally (uv run pytest or pytest)
• Linting passes (uv run ruff check .)
• Documentation updated (if applicable)
• CHANGELOG.rst updated (for user-facing changes)

Testing

Unittests added.
Fork tested manually in an interractive Python shell.

# Commands used to test
uv run pytest tests/test_xxx.py -v

[codecov]

Codecov Report

❌ Patch coverage is 93.02326% with 3 lines in your changes missing coverage. Please review.

Files with missing lines	Patch %	Lines
django_fsm/__init__.py	85.00%	3 Missing ⚠️

Files with missing lines	Coverage Δ
django_fsm/exceptions.py	100.00% <100.00%> (ø)
django_fsm/__init__.py	95.10% <85.00%> (-0.20%)	⬇️

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[pfouque]

Hi @knaperek ,
First of all thanks for contributing.
That's an interesting contribution, I see the global idea but can you share your usage for that?
I'm wondering if raising only the first unmet condition is enough and also if it should be implemented to can_proceed

NB: I pushed a small commit to fix typing issues

[knaperek]

I have a sweeper that tries to transition an object from one state to another, as long as a particular condition allows it. If it does not, it's OK and I want to handle this silently. On the other hand, I'd like to be informed if the transition attempt failed for another reason (e.g. different condition failing or wrong source state etc.).

Using can_proceed before the transition attempt suffers from the TOCTOU issue, while the EAFP approach is more Pythonic.

I deliberately wanted this to be perfectly backwards compatible and not introduce any regressions, including performance. People may have multiple conditions declared on transitions, in a precise order. The current checker uses all() and returns quickly if it finds one that's not passing, skipping the rest of the (potentially heavy) conditions.

[knaperek]

Is there anything else I can do to get this merged?
Cheers.

[pfouque]

First of all, thank you again for this contribution. The implementation is clean, well structured, and easy to review. I really appreciate the care you put into it.

From a technical standpoint, the patch itself looks solid. My hesitation isn’t about quality, but more about validating the underlying use case. I want to make sure we’re solving a problem that is sufficiently common and well-understood before extending the public surface of the project.

As you probably noticed, the community around django-fsm-2 isn’t as active as I’d like, so feedback sometimes takes a bit of time. I’d prefer to let this PR sit a little longer to see whether others chime in with real-world scenarios. Patience here is more about stewardship than doubt in your work.

That said, I’m genuinely interested in your concrete use case. Would you be willing to share a small example of how your code looks today, and how it would look after merging this change? A before/after snippet would be extremely helpful.

I’m currently thinking about enriching the documentation with more “best practices” and possibly a small cookbook section. Real-world examples like yours would be perfect material for that.

On the concurrency side: what you’re addressing does touch the classic TOCTOU category of issues. In many cases, this can be mitigated by using a proper transaction boundary and/or row-level locking (select_for_update).

EAFP is indeed the more Pythonic style, but in distributed systems or concurrent workflows, it sometimes just shifts the problem. And sweepers — while pragmatic (I use them too sometimes) — are often compensating for something happening outside a proper transactional boundary. So I’d like to better understand whether your use case is:

• primarily about ergonomics,
• primarily about correctness under concurrency,
• or compensating for an architectural constraint elsewhere.

None of this blocks the technical quality of the PR — I just want to ensure we evolve the project in a way that remains coherent and minimal.

Thanks again for the thoughtful contribution 🙏

[dwasyl]

@pfouque For what it's worth, this improvement would be a debugging help to us in a few applications. There are some concurrency issues we've run into and just in solving some bugs but having a little more context in the NotAllowed exception would be helpful.

[pfouque]

Hi @dwasyl

I’m a bit hesitant to add this to the core API.

When a project needs detailed information about why a transition cannot proceed, my impression is that this is usually better handled in project code by moving the relevant checks into the transition function itself. That allows the project to either raise a custom exception deriving from TransitionNotAllowed, or emit a log message with the exact debug context it needs.

This keeps the default exception small and stable, while still giving an escape hatch for applications with more advanced debugging/reporting requirements.

It also avoids making assumptions about conditions, which are arbitrary callables and do not always map cleanly to a meaningful public error payload.

More broadly, I’m about to introduce some best practices in the documentation, and one of them is to keep conditions “light”. Conditions are evaluated every time available transitions are listed, so putting non-trivial logic there can have unintended side effects (performance, unexpected behavior, etc.).

In many cases, the conditions parameter is mostly a shortcut to run a check before the transition starts. Moving that logic into the transition function itself often makes little difference in practice (except that the pre-transition signal will be triggered), while giving more flexibility on how to handle failures.

For example:

 class Order(models.Model):
     state = FSMField(default="new")

-    @transition(field=state, source="new", target="paid", conditions=[can_be_paid])
-    def pay(self):
-        pass
+    @transition(field=state, source="new", target="paid")
+    def pay(self):
+        if not self.can_be_paid():  # or can_be_paid(self)
+            raise PaymentNotAllowed("Order cannot be paid due to ...")
+        # or: logger.debug("Payment blocked because ...")
+        ...

[knaperek]

Thanks for your input @pfouque.

It is true that TOCTOU issues can, in general, be avoided with explicit lock or similar pessimistic locking patterns. Sometimes, it is an overkill though - from either performance or just code length and clarity point of view.

I have already introduced the concept of optimistic locking, piggy-backed on the status field, a couple years ago into this library. The neat property and main selling point has been its performance and general overhead characteristics - no added fields or extra queries required, just a tiny bit of logic. And I've been using it heavily for ages with zero issues and perfect feedback from other team members as well.

Similarly, the extra context I'm proposing to add to the exception here has virtually no overhead, but provides optionality. It is also conceptually native to Python or common 3rd party libraries to provide extra context in exceptions. And it makes the code shorter, more DRY, and even faster - following the EAFP principle.

If we were to test all the conditions before the attempt to make the transition, we'd need to either:

1. introduce pessimistic locking that comes at cost, and may be an unjustified overkill if 99+% the cases pass. Not to mention that sometimes it may be trickier to make a 100% reliable pessimistic locking with acceptable performance characteristics. And we're not even considering conditions requiring external checks. Just a damn simple time condition (i.e. allow to change a state back only within 1 hour since... - you can check the time before you enter the critical section, but when FSM evaluates the condition, the time will have changed since).
2. In the end you may often end up handling the exception anyway. And depending on the context you may want to know the reason, because there could be multiple conditions and checks and exception handling logic may need to take a different path depending on the root cause.
   For the popular django-fsm-admin extension, I can imagine it would also be nicer to show the reason for a failed transition - or at least have the ability.

I believe the arguments for this are real and not that hard to imagine in any slightly more complex FSM. We really have nothing to loose but gain with this little improvement.

[pfouque]

Hi @knaperek — thanks again for the work on tracking failed conditions. I built a small follow‑up on top of your contribution to improve exception management while keeping compatibility:

• Kept TransitionNotAllowed as the base so existing handlers still work.
• Added NoTransition for “no transition from state” and InvalidTransition for “method not valid for current state,” so callers can distinguish these cases without string‑matching.
• I also added TransitionConditionsUnmet (my addition, inspired by your work) to avoid optional context and make the unmet‑condition case explicit.
• Updated tests to assert the more specific subclasses where appropriate.

I’d love your feedback on the hierarchy and naming — does this fit the intent of your original change?