search

gnustavo / JIRA-REST

Thin wrapper around Jira's REST API
https://metacpan.org/release/JIRA-REST/
17 stars 17 forks source link

Adjust regex to check path #9

Closed evanernest closed 7 years ago

evanernest commented 7 years ago

I should have caught this while we were discussing my prior PR #6 :

I changed the regex here to match more endpoints when specifying the full endpoint. Examples of valid endpoints that the current regex wouldn't allow:

https://instance-name.atlassian.net/wiki/rest/api/latest (Confluence endpoint, '/wiki' precedes '/rest')

https://jira.example.com/rest/agile/latest (Software endpoint, ends with '.../agile/latest', instead of '.../api/latest')

https://jira.example.com/rest/servicedeskapi/latest (Software endpoint, ends with '.../servicedesk/latest', instead of '.../api/latest')

The new regex simply requires the endpoint to contain "/rest" and end in "/VERSION" or "/latest".

Sorry again for not catching before. My app tests didn't catch this. I wrote endpoints.t to test in the future.

gnustavo commented 7 years ago

Hi Evan. The way it is now you can use JIRA::REST to talk any of the JIRA REST APIs. You only have to pass the "absolute" resource path to the GET/POST/PUT/DELETE methods. For example:

$i = $jira->GET("/rest/agile/1.0/issue/$key");
$sla = $jira->GET("/rest/servicedeskapi/request/$key/sla");

Relative resource paths are only allowed for the "core" JIRA REST API. That's why I'm only checking for /rest/api/(?:latest|\d+) in the constructor, to see if the user wants a specific version of the core API.

If we let the user specify a prefix for "any" API we make the JIRA::REST object be specific to that particular API, which I don't think is a good idea.

evanernest commented 7 years ago

Ahhhhh. Sorry. I'm dumb. I misinterpreted. Your way is indeed better.

How about this change?

https://github.com/evanernest/JIRA-REST/commit/ed2c81bed477c1dc0f354fd35336dfa08d3b4c49

The pesky Confluence API puts "/wiki" before "/rest" in the path.

gnustavo commented 7 years ago

To use Confluence's API? I'm not sure... Confluence isn't JIRA which means we're beginning to stretch the scope of JIRA::REST a little too much I'm afraid.

On the other hand, if it works as is, perhaps the best idea would be to rename it to Atlassian::REST or something like this.

I'll have to think about it a little more.

evanernest commented 7 years ago

Ah. That's a fair point. Got my umbrella terms confused. It does seems to work as is, but it isn't JIRA.

mannih commented 7 years ago

I just found this little thread here and I wonder whether I understand things or not.

  1. I think our company's JIRA is sitting behind a "/jira" url path. Thus I'd like to create a JIRA::REST instance with a URL like "https://some.host/jira/". This fails because of the regular expression you guys have been discussing.

  2. Skimming the source code, I get the idea that JIRA::REST should work with our url once I get that url path behind the constructor.

  3. Maybe the API version should be a separate option that is not deducted from the path?

gnustavo commented 7 years ago

Hi @mannih. You're right... the change in 235197f7 messed up with the JIRA installations that use a path prefix. The main problem is that previously I checked the URL suffix with no left-anchor and then I put an anchor there.

I'm thinking about how best to solve this. Please, bear with me.

gnustavo commented 7 years ago

@mannih , I think 3745bab solves the problem.

In retrospect, It would be best to specify the default API with a different option, but to be backwards compatible I have to admit it being specified in the URL.

What do you think?

mannih commented 7 years ago

@gnustavo : I think at least three things:

  1. It's awesome to hear back from you so soon.
  2. Deleting that single anchor would solve my little problem really fast ;-)
  3. packy's pull request might open the door for a nicer way to specify API version and URL.
gnustavo commented 7 years ago

@mannih , yes, I want to integrate @packy's pull request soon. I'm going to merge this fix into its pull request and review it once more before cutting a new release.

gnustavo commented 7 years ago

I just released JIRA-REST-0.016-TRIAL which incorporates this pull request.

Please, let me know if you notice any problems in it.

mannih commented 7 years ago

The trial release works perfect for me. There's a minor "usability" issue for people like me using a path prefix: specifying "https://host/jira/" will not work and lead to 404s down the road. But if I leave off the trailing slash and do "https://host/jira", every thing is fine.

Thank you!

evanernest commented 7 years ago

This also works for me. Thanks!

gnustavo commented 7 years ago

Sorry @mannih , I forgot about your comment regarding the trailing slashes. I just committed a change that should fix this. It'll be part of the next release.