Skip to content

Fix fastapi root path o2m m2m search#962

Closed
bikmetle wants to merge 4 commits intosmithyhq:mainfrom
bikmetle:fix-fastapi-root-path-o2m-m2m-search
Closed

Fix fastapi root path o2m m2m search#962
bikmetle wants to merge 4 commits intosmithyhq:mainfrom
bikmetle:fix-fastapi-root-path-o2m-m2m-search

Conversation

@bikmetle
Copy link
Copy Markdown

fix the search in one-to-many or many-to-many field if fastapi root_path is set

app = FastAPI(
root_path="/api",
)

self.favicon_url = favicon_url

if hasattr(self.app, "root_path"):
self.base_url = self.app.root_path + base_url
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we do urljoin to make sure starting/ending slashes or joined properly?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe

    if hasattr(self.app, "root_path"):
        parts = [p.strip("/") for p in [self.app.root_path, base_url] if p.strip("/")]
        self.base_url = "/" + "/".join(parts)

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

or better

self.base_url=urljoin(app.root_path+"/", base_url.lstrip("/"))

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think urljoin will do that so:

self.base_url=urljoin(app.root_path, base_url)

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i've tried different paths, the only working way is to do

self.base_url=urljoin(app.root_path+"/", base_url.lstrip("/"))

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you are right, probably it should be changed to this:

self.base_url=os.path.join(app.root_path, base_url)

Also a few other points:

  • tests should be added
  • there's an existing code using urljoin which I think should be changed to os.path.join

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@bikmetle reminder

Copy link
Copy Markdown
Member

@mmzeynalli mmzeynalli left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@mmzeynalli
Copy link
Copy Markdown
Member

@aminalaee

@aminalaee
Copy link
Copy Markdown
Collaborator

@mmzeynalli I was waiting for this comment to be resolved. I think it's a small change but we should also add tests for it

@mmzeynalli mmzeynalli added the waiting-for-feedback Waiting feedback/answer/updates from contributor label Apr 4, 2026
@mmzeynalli
Copy link
Copy Markdown
Member

@JartanFTW your PR #996 addresses this, right?

@JartanFTW
Copy link
Copy Markdown

@JartanFTW your PR #996 addresses this, right?

From my understanding, yes.

@mmzeynalli
Copy link
Copy Markdown
Member

@bikmetle I'm closing this PR. Thanks for your work, but #996 handles this in a broader way.

@mmzeynalli mmzeynalli closed this Apr 6, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

waiting-for-feedback Waiting feedback/answer/updates from contributor

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants