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

Service class architecture silently ignores duplicate routes #2155

Open
chris48s opened this issue Oct 7, 2018 · 2 comments
Open

Service class architecture silently ignores duplicate routes #2155

chris48s opened this issue Oct 7, 2018 · 2 comments
Labels
core Server, BaseService, GitHub auth developer-experience Dev tooling, test framework, and CI

Comments

@chris48s
Copy link
Member

chris48s commented Oct 7, 2018

This is a bit of a contrived example, but if I declare something like:

class VersionService1 extends BaseJsonService {
  //stuff

  static get url() {
    return {
      base: 'someroute/v',
      format: '(.+)',
      capture: ['param'],
    }
  }
}

class VersionService2 extends BaseJsonService {
  //stuff

  static get url() {
    return {
      base: 'someroute/v',
      format: '(.+)',
      capture: ['param'],
    }
  }
}

module.exports = {
  VersionService1,
  VersionService2,
}

VersionService1 and VersionService2 are both trying to mount on the same route. When we start the server, one wins and one loses (I guess the order we register the services in is important here) but it doesn't throw an error. In the example above, it is pretty easy to debug, but once you've got services spread across multiple files this condition could become quite hard to detect.

It would be a nice safeguard if we added a check when we register the service classes which throws an error if we have declared any duplicate or conflicting routes.

@chris48s chris48s added the developer-experience Dev tooling, test framework, and CI label Oct 7, 2018
@paulmelnikow
Copy link
Member

Yea, good shout. We could check for duplicate base URLs.

On rare occasions the regexes might be the only thing that differs, so we'd need a way to ignore the warning on those.

@paulmelnikow
Copy link
Member

It occurred to me there's something else we could do here. We could collect all the example URLs, and then run them through each service and make sure they only match one of them. It shouldn't be too hard to add negative lookaheads to prevent false positives.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
core Server, BaseService, GitHub auth developer-experience Dev tooling, test framework, and CI
Projects
None yet
Development

No branches or pull requests

2 participants