[IMP] fastapi: As we have route_group, we could use it

[etobella]

This allows us to set the route_group.

Really interesting in conjunction with OCA/web-api#149

[OCA-git-bot]

Hi @lmignon,
some modules you are maintaining are being modified, check this out!

[lmignon]

Thank you for the proposal @etobella It's interesting. Nevertheless. The field you add to the fastapi addon is useless while the the endpoint_route_handler_filter is not installed. It therefore depends on the endpoint_route_handler_filter addon. Since it depends on an additional addon, I prefer to declare-it into a glue addon to keep a fine grained approach and ease traceability.

[etobella]

I understand your comment @lmignon , however, the field is defined in the dependency, the change is 4 lines of code and it has no effect (2 additions and 2 changes). Also, with the configuration of endpoint_route_handler, it was expected that fastapi should send it. If we add a new module, we are adding much more code to the code base (around 500 probably between readmes, html and so on, only in python and xml it will be around 50 at least) for something that has no harm if we have it in the main module and should be there.

Evenmore, if we want a new module we require a change in this module to add a hook, so the code added in the code base is similar (1 edition at least and 3-4 additions of the hook function)

If you want, we can make this field hidden in the view if the problem is visual, but adding it is minimal and has no harm.

Please reconsider it to avoid adding too much code in the database.

[CristianoMafraJunior]

LGTM

[ValentinVinagre]

Lgtm 🤘🏻

[lmignon]

Thanks for working on this.

I understand that creating a dedicated addon may feel like extra work for what looks like "just a couple of fields". However, I don't think adding fields whose only purpose is to support an addon outside of this dependency chain is the right trade-off.

Keeping these fields in fastapi has several drawbacks:

• it breaks the traceability of where this functionality actually comes from;
• it prevents having integration tests covering the complete feature;
• it makes future maintenance harder, as these fields appear unused from the perspective of fastapi itself and could easily be considered dead code during a refactoring or migration;
• it also introduces API surface that fastapi doesn't need by itself.

For these reasons, I'd rather keep fastapi focused on its own responsibilities and have the integration live in a dedicated addon. That addon can depend on both projects, making the dependency explicit and allowing the feature to be tested, documented, promoted and maintained as a whole.

While this does require creating an additional addon, I believe the extra effort is negligible compared to the long-term benefits in terms of clarity, maintainability, and reliability.

[etobella]

Thanks for the detailed explanation — I'd like to clarify something important that I think changes the calculus here.

route_group isn't being added for some addon outside our dependency chain. It's expected by endpoint_route_handler, which fastapi already depends on. So this isn't new coupling to an external addon — it's fastapi correctly fulfilling a field that its existing dependency already expects. You are asking for a glue-module for fulfilling something that we forget to do in fastapi. The glue-module argument assumes we're bridging two independent addons that don't otherwise know about each other, but that's not the case: endpoint_route_handler is already in the dependency graph.

On the specific points:

• Traceability: a help string on the field referencing endpoint_route_handler documents this clearly, and arguably more directly than a separate module would, since the relationship is between fastapi and a module it already depends on.
• Integration tests: since the field just satisfies what endpoint_route_handler expects, behavior tests belong there, not in a new glue addon that would sit awkwardly between two modules that are already connected.
• Dead code risk: mitigated by the help text; this isn't a speculative field, it's closing a gap between fastapi and a dependency it already has. Also, the field will be used in the make_rule, not a field used nowhere.
• API surface: this is exposing something the existing dependency chain already anticipates, not introducing new external surface.

Just to add, adding this glue module is adding a lot of code for just a small functionality. The added code is 5 lines of code, adding a module it has a much bigger cost in lines of code ( around 1000 lines of code for a simple module -- OCA/partner-contact#1947 ).

To provide more information, you can see that endpoint module has this route_group field for the same purposes. https://github.com/OCA/web-api/blob/18.0/endpoint/views/endpoint_view.xml#L51

Given that, I don't think a dedicated addon is justified here — it would be gluing together two modules that are already directly related, adding a layer of indirection rather than removing coupling. I'll add the help text/comment for traceability if that resolves the concern or hide the field if the problem is seeing it there.

[lmignon]

Thanks for taking the time to explain your reasoning.

However, I still don't share the same view on this.

From my perspective, this field has no functional effect unless your new addon is installed. While it is added through a hook defined by fastapi/endpoint_route_handler, its semantics and expected behavior are entirely driven by an addon that is not part of fastapi's dependency chain. That's exactly why I think it should live in an integration addon.

My concern is not about the implementation itself, but about keeping the dependency graph explicit. Looking only at fastapi, there is no way to understand why this field exists or whether it is actually required. The fact that another addon gives meaning to it is, in my opinion, precisely what should be expressed through a dedicated integration module.

Regarding the amount of code, I don't think counting generated files (translations, manifests, boilerplate, etc.) is a relevant metric. The cost of an additional addon is relatively small, whereas the architectural benefits—clear dependencies, better traceability, and easier long-term maintenance—are much more valuable. Otherwise, we could use the same argument to justify moving more and more integration code into base modules instead of keeping a modular design.

So, while I understand your point of view, I'd still prefer to keep this integration in a dedicated addon.

[pedrobaeza]

AFAIK, fastapi module is a base one, so you require to put another module on top with the desired features, so adding this new option in the base one is reasonable and will be used by further modules on top. Apart from not being a new option per se, but only exposing it from the underlying dependency. Not having it here will provoke several concurrent implementations of the same feature (like it's happening with endpoint), possible collapsing between them.

The risk of having dead code in base modules is there, as they are not self-sufficient, but in this case being a new argument in some functions reduce it, and may be covered by mocked tests if required.

Thus, I vote for including it in the existing module.

[sbidoul]

Since this PR does not add a new dependency in the manifest, I initially thought route groups were a concept from Odoo core that I did not know. That alone is the proof that something is not right with this PR.

Since the concept of route group is introduced by the endpoint_route_handler_filter, it is absolutely clear to me that a glue module is needed.

Just to add, adding this glue module is adding a lot of code for just a small functionality

Not that much more code, to be honest, but sure, but that's how Odoo modularity works.

[lmignon]

Thanks everyone for your feedback.

I understand your reasoning, but I still don't agree. For me, this is an architectural decision rather than an implementation detail.

Since I originally designed this module, my goal has always been to keep fastapi focused on concepts that it owns and uses itself. When a piece of code only exists to support an addon outside its own scope, I prefer keeping it in a dedicated integration addon, even if this means creating an additional module.

In this particular case, the field is not required for the normal usage of fastapi. It is adding support for an additional feature provided by another addon. By putting this field directly in fastapi, we introduce an implicit responsibility for a feature that is outside its scope and that fastapi itself does not need. This also means that future maintainers of fastapi will have to consider and preserve this behavior, even if they are not using this feature.

More generally, I consider this separation of responsibilities to be one of the fundamental principles behind Odoo's modularity and the quality of OCA addons. Keeping integrations explicit makes dependencies easier to understand, improves maintainability, and makes it much easier to evolve modules independently over time.

I don't think the amount of boilerplate is a strong argument here. The extra effort is small compared to the long-term benefits of a clean and explicit architecture.

I understand that others would make a different trade-off, and I respect that. However, as the maintainer of this module, I'd like to keep following this design principle, as I consistently do across all the modules I maintain or contribute to.

For this reason, I'm going to close this proposal in its current form.

[OCA-git-bot]

Hi @lmignon,
some modules you are maintaining are being modified, check this out!

[etobella]

No!

https://github.com/OCA/web-api/blob/18.0/endpoint_route_handler/registry.py#L81

This fields are declared in enpoint_route_handler, the dependency of this module

[etobella]

I can close, but no because you decide that it depends on a different module. That module was a way to use this field that exists in endpoint_route_handler. It should exist or at least be defined somehow.

At least provide a real reason for not using a field that is declared by the dependency.

[etobella]

Just one last comment. If you want to not accept the change, I can understand, but a change will be required in this module to make the glue module with a diff of the same size than this one. So please, do not close.

[sbidoul]

This fields are declared in enpoint_route_handler, the dependency of this module

Ah, thanks, I see it now. Ok, this makes more sense to me, then.

Since each fastapi endpoint seems to generate a single endpoint rule, so there is a one to one correspondance between a fastapi endpoint and a endpoint rule, it makes sense that properties of endpoint rules can be obtained from a fastapi endpoint.

[simahawk]

Disclaimer: I'm not interested in the filter module (and I'm not sure I like its implementation) but I know a bunch of things about the route handler and why route_group is there 😉

From my POV exposing route_group here is helpful and does not bring any burden. As explained by Enric - it completes the usage of endpoint_route_handler by fastapi.

The purpose of that field is to group registered routes together.
Today if you want to "tag" or "bundle" fastapi routes together, you cannot.

From UI perspective, being a low level technical field you could simply show it only in debug mode -> same experience as of today by default.
From maintenance perspective, just describe its purpose -> nobody will trash it or wonder why the heck is there.

My $0.02.

[lmignon]

@etobella After your last comment, I went back and checked the implementation more carefully. I had overlooked an important point: route_group is already a concept defined by endpoint_route_handler, not something introduced solely for the new addon.

My initial concern was based on the assumption that we were adding a field to fastapi whose only purpose was to support an addon outside its own scope. In that case, I would still consider a dedicated integration addon to be the right design.

Now that I understand route_group already belongs to the underlying dependency, I see the proposal differently. I still don't think the amount of boilerplate is a relevant argument when discussing architecture, but exposing an existing concept from the underlying layer is, in this case, a reasonable design choice.

Thanks for pointing me to that and for the constructive discussion. Sorry for making the review process longer than necessary because I had missed this point.

[lmignon]

@etobella Can you add a help text to describe its usage?

[etobella]

@simahawk I know that that implementation can be improved. It was just a proof of concept.

The idea was that we could filter access to a fastapi endpoint. For example, to allow connection from an internal server but not from external ones. I know that it can be improved, but that was the idea. I am sure that there will be some discussion in that topic in the other PR and I will be happy to find a common ground for everyone.

@lmignon Don't worry, I probably could have been clearer. However, I am happy that we all were able to understand each other. Sorry if I fought too much.

I agree that if the field was defined in my proposed module, a glue module was necessary. However, that was not the case, for that reason I tried to explain it.

[lmignon]

/ocabot merge minor

[OCA-git-bot]

Hey, thanks for contributing! Proceeding to merge this for you.
Prepared branch 19.0-ocabot-merge-pr-634-by-lmignon-bump-minor, awaiting test results.

[OCA-git-bot]

Congratulations, your PR was merged at 49153d6. Thanks a lot for contributing to OCA. ❤️