Skip to content

fix(api): fix incorrect path handling in Langfuse integration#22766

Merged
QuantumGhost merged 7 commits intolanggenius:mainfrom
chenguowei:fix/validate-path
Jul 28, 2025
Merged

fix(api): fix incorrect path handling in Langfuse integration#22766
QuantumGhost merged 7 commits intolanggenius:mainfrom
chenguowei:fix/validate-path

Conversation

@chenguowei
Copy link
Copy Markdown
Contributor

@chenguowei chenguowei commented Jul 22, 2025

Solve the problem of langfuse configuration error resolves #22757

Important

  1. Make sure you have read our contribution guidelines
  2. Ensure there is an associated issue and you have been assigned to it
  3. Use the correct syntax to link this PR: Fixes #<issue number>.

Summary

Screenshots

Before After
... ...

Checklist

  • This change requires a documentation update, included: Dify Document
  • I understand that this PR may be closed in case there was no previous discussion or issues. (This doesn't apply to typos!)
  • I've added a test for each change that was introduced, and I tried as much as possible to make a single atomic change.
  • I've updated the documentation accordingly.
  • I ran dev/reformat(backend) and cd web && npx lint-staged(frontend) to appease the lint gods

@dosubot dosubot bot added the size:XS This PR changes 0-9 lines, ignoring generated files. label Jul 22, 2025
@crazywoola
Copy link
Copy Markdown
Member

Please fix the tests.

@Nov1c444
Copy link
Copy Markdown
Contributor

Maybe It's the better way to fix the issue.
change the validate_endpoint_url method in config_entity.py
image

@Nov1c444 Nov1c444 self-requested a review July 23, 2025 02:51
@chenguowei
Copy link
Copy Markdown
Contributor Author

The validate_url function itself only verifies whether the url is correct and should not change the original url. This is the definition of a validation function.

@dosubot dosubot bot added size:S This PR changes 10-29 lines, ignoring generated files. and removed size:XS This PR changes 0-9 lines, ignoring generated files. labels Jul 23, 2025
@QuantumGhost
Copy link
Copy Markdown
Contributor

@chenguowei I agree with your principle that validate_url should focus solely on URL validation without modifying the input. However, I'm concerned about the potential impact of this change on the existing codebase.

The validate_url function is used across multiple locations in the codebase, and changing its behavior from returning a normalized URL to returning the original URL could introduce breaking changes in other components that depend on the current normalization behavior.

I suggest a more conservative approach:

  1. Introduce validate_url_with_path for cases where path preservation is needed (like Langfuse configuration)
  2. Mark validate_url as deprecated
  3. Gradually migrate existing usages to the appropriate function based on their requirements.

- Add a note about its behavior of not retaining the `path` component
  in its return value.
- Mark the function as deprecated (as in most situations,
  we need to retain the `path` component)
- Revert changes about `validate_url` and its test cases.
The validation function should keep the original path instead
of returning a modified result.
Call `validate_url_with_path` directly in `LangfuseConfig`, instead of
calling `cls.validate_endpoint_url`. (The later introduces
a tight coupling between `LangfuseConfig` and `BaseTracingConfig`)
@QuantumGhost QuantumGhost changed the title Remove the truncation logic in the validate process fix(api): fix incorrect path handling in Langfuse integration Jul 28, 2025
The `BaseTraceInstance` requires the configuration to be a subclass of
`BaseTracingConfig`.
@dosubot dosubot bot added the lgtm This PR has been approved by a maintainer label Jul 28, 2025
@QuantumGhost QuantumGhost merged commit fce126b into langgenius:main Jul 28, 2025
7 checks passed
@Hammad663
Copy link
Copy Markdown

Not interested in your artifactsi

Copy link
Copy Markdown
Contributor Author

@chenguowei chenguowei left a comment

Choose a reason for hiding this comment

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

by modifying case by case in this way, I understand that other subsequent modifications may also need to be made successively

@QuantumGhost
Copy link
Copy Markdown
Contributor

@chenguowei Yes, I understand your concern about potential cascading changes. However, it appears that other integrations are working fine with the current approach. Let's keep it as-is for now and monitor if similar issues arise elsewhere.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

lgtm This PR has been approved by a maintainer size:S This PR changes 10-29 lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

The Host and path configured by Langfuse have been truncated.

5 participants