Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add an option to disable duplicate path mapping detection #1283

Closed
trustin opened this issue Jul 12, 2018 · 8 comments
Closed

Add an option to disable duplicate path mapping detection #1283

trustin opened this issue Jul 12, 2018 · 8 comments
Milestone

Comments

@trustin
Copy link
Member

trustin commented Jul 12, 2018

As proposed by @anuraaga at #529 (comment)

@trustin trustin added this to the 0.68.0 milestone Jul 12, 2018
@anuraaga
Copy link
Collaborator

Also I was thinking this is probably cleanest as adding serviceIfAbsent methods. It would be too much to add all the convenience method versions but just two for PathMapping and ServiceWithPathMapping should be complete and enough since it's just an advanced usage.

@trustin
Copy link
Member Author

trustin commented Jul 12, 2018

Duplicate detection can never be perfect, so serviceIfAbsent() would not work as expected - it may be added even if there is actually a duplicate mapping. 😭

@anuraaga
Copy link
Collaborator

Ah yeah that's true. Actually I guess it means on the flip side it could only have the one version that takes an exact path String since it's the only duplicate we detect right now IIUC

@trustin
Copy link
Member Author

trustin commented Jul 12, 2018

Actually I guess it means on the flip side it could only have the one version that takes an exact path String since it's the only duplicate we detect right now IIUC.

We also check duplicates for all trie-based path mappings. Regex-based mappings complicate the matter.

I see a few options here:

  • Add an option that disables duplicate mapping detection.
    • We may also want to add a method the checks if there's duplicate mapping.
  • Do not perform duplicate mapping detection at all, i.e. revert the PR.
    • .. because it's imperfect and a user will notice eventually? /cc @imasahiro
  • Allow a user to specify the callback for duplicate services.
    • The default callback would be to raise an exception or log a warning message.
    • A user could choose to do nothing in the callback.

Thoughts?

@minwoox
Copy link
Member

minwoox commented Jul 12, 2018

I think we need to catch the duplicate paths as much as possible although it cannot be perfect.
So the user would notice when he/she start the server.
I like the way that allow a user to specify the callback and the default callback is raising an exception.

@hyangtack
Copy link
Contributor

I agree with @minwoox 's comment. Option 1 or 3 sounds good to me.

@anuraaga
Copy link
Collaborator

Yeah the callback sounds good too. Maybe it can be something on the exception itself, something like DuplicatePathException#applyAnyways(), and users just catch the exception if they care about applying anyways.

@trustin
Copy link
Member Author

trustin commented Jul 12, 2018

Let me schedule this for the release after the next release since we are releasing soon.

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

No branches or pull requests

4 participants