Skip to content

Conversation

damnedest
Copy link
Contributor

Fixes #361

What changed:

Replaced reflect.Type.String()-based type identification with a robust builder that uses PkgPath() + Name() for named types, after unwrapping pointers and inspecting constructor return types. Updated GetType and GetReturnType accordingly.

Why:

To prevent resolution collisions between handlers/middlewares that share the same package and type names under different import paths (e.g., admin vs public), which led to the wrong handler being invoked.

How to reproduce the bug (before the fix):

  1. Define two handlers with identical package and type names in different modules:
    • internal/ports/http/public/endpoints/web_auth_login_v1.Handler
    • internal/ports/http/admin/endpoints/web_auth_login_v1.Handler
  2. Register them via generated group registrations.
  3. Call POST /admin/web/auth/login/v1 and observe that the public handler implementation can be executed.

How to verify the fix:

  1. Start the server with debug logging enabled.
  2. Call both:
    • POST /admin/web/auth/login/v1
    • POST /public/web/auth/login/v1
  3. Confirm each endpoint triggers the correct handler from its respective package and that logs enumerate unique identifiers for admin vs public handlers.

@damnedest
Copy link
Contributor Author

If it's ok, I will make separate PR for all modules with this functions.

Copy link

codecov bot commented Aug 31, 2025

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 98.41%. Comparing base (9e24793) to head (579f361).

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #362      +/-   ##
==========================================
+ Coverage   95.17%   98.41%   +3.24%     
==========================================
  Files         177       15     -162     
  Lines        6999      819    -6180     
==========================================
- Hits         6661      806    -5855     
+ Misses        237        9     -228     
+ Partials      101        4      -97     
Flag Coverage Δ
fxconfig ?
fxcore ?
fxcron ?
fxgrpcserver ?
fxhttpclient ?
fxhttpserver 98.41% <100.00%> (+0.02%) ⬆️
fxmcpserver ?
fxmetrics ?
fxorm ?
fxsql ?
fxtrace ?
fxvalidator ?
fxworker ?
grpcserver ?
httpclient ?
httpserver ?
orm ?
sql ?
trace ?
worker ?

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@ekkinox
Copy link
Contributor

ekkinox commented Aug 31, 2025

Hi, thanks for the contribution 🙏

There's an issue with the CI linter, I check as soon as I can, then I release your change 👍

@ekkinox
Copy link
Contributor

ekkinox commented Sep 1, 2025

@damnedest, I tried to fix linter in #363, it's on main.

Could you please rebase your branch and push again ?

@damnedest damnedest force-pushed the fix/fxhttpserver-reflection-type branch from 579f361 to 4607642 Compare September 1, 2025 08:41
@damnedest
Copy link
Contributor Author

@ekkinox done

@ekkinox ekkinox changed the title fix(fxhttpserver): HTTP handlers/middlewares are mixed up when package/type names match across different import paths (type identification collision) fix(fxhttpserver): Fix HTTP handlers/middlewares dependency injection signature Sep 1, 2025
@ekkinox ekkinox merged commit 2042e33 into ankorstore:main Sep 1, 2025
2 checks passed
@damnedest
Copy link
Contributor Author

@ekkinox Do you mind if I make same PRs in other components with a similar change? Each component - one PR?

@ekkinox
Copy link
Contributor

ekkinox commented Sep 1, 2025

@ekkinox Do you mind if I make same PRs in other components with a similar change? Each component - one PR?

Yokai release is based on a bot that requires modules (fxhttpserver, fxgrpcserver, etc) to each have a dedicated PR in order to correctly tag/release them independently. Like you did for fxhttpserver.

Before starting, in which other module do you want to make a similar change?

@damnedest
Copy link
Contributor Author

@ekkinox Same implementation of GetType/GetReturnType exists in:

@ekkinox
Copy link
Contributor

ekkinox commented Sep 1, 2025

Ok 👌

If you have the motivation to make a PR for each one, it'll be very welcomed 🤩

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

HTTP handlers/middlewares are mixed up when package/type names match across different import paths (type identification collision)
2 participants