Page MenuHomePhabricator

Don't handle JIRA/Asana URIs with anchors or query parameters in a special way (with Doorkeeper)
ClosedPublic

Authored by epriestley on Jun 19 2019, 5:46 PM.

Details

Summary

Ref T13291. See PHI1312. Currently, if you link to a JIRA or Asana issue with an anchor (#asdf) or query parameters (?a=b), we:

  • treat the link as an external object reference and attempt a lookup on it;
  • if the lookup succeeds, we discard the fragment or parameters when re-rendering the rich link (with the issue/task title).

Particularly, the re-rendering part uses the canonical URI of the object, and can discard these parameters/fragments, which is broken/bad.

As a first pass at improving this, just don't apply special behavior for links with anchors or parameters -- simply treat them as links.

In some future change, we could specialize this behavior and permit certain known parameters or anchors or something, but these use cases are likely fairly marginal.

Test Plan

Before:

After:

Diff Detail

Repository
rP Phabricator
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

epriestley created this revision.Jun 19 2019, 5:46 PM
epriestley requested review of this revision.Jun 19 2019, 5:47 PM

This is just short-circuiting the whole external object lookup process, right? Is it significantly more work to just fix the lookup process to preserve these fragments?

It's meaningfully more work, but the bigger thing stopping me is that I'm not sure what the best display behavior is.

In my screenshots, the JIRA provider was a temporary instance that got shut down, but normally you type this:

https://jira.example.com/browse/XXX-123

...and we ultimately render this, after an Ajax request to do the API call to make sure you can see the object:

( XXX-123 Order More Copier Paper )

If you type this instead:

https://jira.example.com/browse/XXX-123?x=y&a=b#bottom

...for some arbitrary query string and fragment, what should we render?

If it's the same thing, that seems possibly confusing (two different links have the same visual rendering).

If we know what the parameter means we can render ( XXX-123 Order More Copier Paper · Comment #33 ) or something similar, but, in the general case, we can't interpret the parameters in any way.

I think we'd have to render something like this:

( XXX-123 Order More Copier Paper · ?x=y&a=b#bottom )

...which seems weird/confusing and sort of worse than just leaving the link alone? Do you see another approach?

If it's the same thing, that seems possibly confusing (two different links have the same visual rendering).

This seems totally reasonable to me? Also likely to be less surprising than suddenly losing external object rendering just because you added a URL param. Also-also, just rendering the parts of the link we understand puts us in a cleaner place for eventually adding additional per-param rendering when someone complains that two different links have the same visual rendering.

I guess my assumption is that ?a=b may, in the general case, completely change the meaning of the link. Although I think this has mostly been purged from the web now, an example was #! fragments before the widespread use of the history API, where https://example.com/a/#!b actually meant "Page B", not "Page A", and labeling the link "Page A" would be misleading.

Do you think ( XXX-123 Order More Copier Paper ) is still a reasonable rendering if the parameter is something like ?download=123 and that means "Download Attached File 123" and clicking the link doesn't take you to XXX-123 at all?

Also, practically, I can't actually figure out how to get either modern JIRA or Asana to generate any URI with any parameters or fragments at all. Neither appears to support linking directly to a comment.

When you click timestamps in JIRA it toggles all times on the page between absolute and relative, but does not generate a permanent link.

Uhhh...

https://jira.atlassian.com/browse/JRASERVER-44899

amckinley accepted this revision.Jun 19 2019, 7:21 PM

Do you think ( XXX-123 Order More Copier Paper ) is still a reasonable rendering if the parameter is something like ?download=123 and that means "Download Attached File 123" and clicking the link doesn't take you to XXX-123 at all?

I think this falls into the category of "first, tend to thine own garden". Doorkeeper is already "doomed" in the sense that external systems will never present a completely reasonable external interface, so saying "what if some system does something nonsensical" seems like sacrificing the common case (the whole point of Doorkeeper) to support an edge case.

I don't feel militantly about disabling Doorkeeper in the presence of URL params vs suppressing the params during rendering, but either solution is better than the current behavior so I'm accepting this. I think I still tend to slightly prefer rendering potentially-misleading links because it moves us forward, and someone will complain if they find an egregious example.

When you click timestamps in JIRA it toggles all times on the page between absolute and relative, but does not generate a permanent link.

Some rules were meant to be broken 😎

This revision is now accepted and ready to land.Jun 19 2019, 7:21 PM

(Also, now that I've read PHI1312, I mind this^^ implementation less since that was the actual support request anyway).

If JIRA links are mostly/all in the form ?page=com.jira.jira.plugin.extension.gateway-to-realms-of-wonder&exec=rm -rf / that might also mean that "links which don't really link to the task" are common and that "task + params" is more like "portal to arbitrary plugin behavior", although I'm not having much luck digging up more information about this by Googling.

I'm just going to land this as-is pending feedback on actual use cases, anticipating a followup to do parameter/anchor support if some reasonable #comment-123 URI exists in either external system. Then it will at least be easy for us to link-and-preserve-parameters or decline-to-link and we can figure out whatever else arises on a case-by-case basis.