Add test example of unquoted rel. #287

jalcine · 2019-03-27T22:45:19Z

This is the issue noticed in #286. It's also a real-world example held at https://webmention.rocks/

jalcine · 2019-03-27T22:52:11Z

Already highlighted in first failure at https://travis-ci.org/teamon/tesla/jobs/512263620

codecov · 2019-03-27T23:22:36Z

Codecov Report

Merging #287 into master will decrease coverage by 0.45%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master     #287      +/-   ##
==========================================
- Coverage    95.4%   94.94%   -0.46%     
==========================================
  Files          23       23              
  Lines         457      455       -2     
==========================================
- Hits          436      432       -4     
- Misses         21       23       +2

Impacted Files	Coverage Δ
lib/tesla.ex	`83.33% <0%> (-4.17%)`	⬇️
lib/tesla/middleware/logger.ex	`88.63% <0%> (-0.26%)`	⬇️
lib/tesla/middleware/timeout.ex	`100% <0%> (ø)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 52d3a3b...390a267. Read the comment docs.

codecov · 2019-03-27T23:22:37Z

Codecov Report

❗ No coverage uploaded for pull request base (master@3195e25). Click here to learn what that means.
The diff coverage is 75%.

@@           Coverage Diff            @@
##             master    #287   +/-   ##
========================================
  Coverage          ?   95.2%           
========================================
  Files             ?      23           
  Lines             ?     459           
  Branches          ?       0           
========================================
  Hits              ?     437           
  Misses            ?      22           
  Partials          ?       0

Impacted Files	Coverage Δ
lib/tesla/middleware/decode_rels.ex	`88.88% <75%> (ø)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 3195e25...d08c0b5. Read the comment docs.

teamon · 2019-04-01T14:38:17Z

lib/tesla/middleware/decode_rels.ex

-    |> List.to_tuple()
+    case Regex.run(~r/\A<(.+)>; rel=(.+)\z/, item, capture: :all_but_first) do
+      nil ->
+        {}


iex(3)> [{}] |> Enum.into(%{}) ** (ArgumentError) argument error (stdlib) :maps.from_list([{}]) (elixir) lib/map.ex:174: Map.new/1

Empty tuple is not a good return value, this should be either ok/error tuple or a list

What do you suggest? Something like {:ok, []}?

It depends on what is valid and what not, in this case I think it should be an empty set of rels

teamon · 2019-04-01T14:39:52Z

lib/tesla/middleware/decode_rels.ex

+      link_rel ->
+        link_rel
+        |> Enum.reverse()
+        |> List.replace_at(0, String.trim(List.last(link_rel), "\""))


This one is quite hard to comprehend. Also, the first regex will match everything, it should probably have some stop point instead of .+

We should either parse key="something" or key=something, i.e. not accepting xxx="somthing (only one quote)

By the first regex, you mean the \A<(.+)> bit? It doesn't seem to do that in the tests; especially since we discard the first matching result in the options.

Do you have suggestions on changes?

This bit rel=(.+) - it will match rel="hello while it should match only either rel=hello or rel="hello"

teamon · 2019-04-24T16:25:59Z

@jalcine 👋

jalcine · 2019-04-24T23:56:09Z

Thanks for bumping this. I’ll be checking out your changes today!

(Originally published at: https://v2.jacky.wtf/post/ad63a126-378a-461e-991f-a4ad8b05edba)

teamon · 2019-05-31T10:48:39Z

Hey @jalcine, what's the status of this PR?

teamon · 2019-07-22T11:32:12Z

@jalcine 👋

teamon · 2019-09-05T11:52:11Z

@jalcine ping

jalcine · 2019-09-13T19:58:08Z

@jalcine ping

Yo, my bad. I can take a look at this over the weekend - been busy with work. I've actually been working off my fork and I'd love to have this mainlined.

jalcine · 2019-10-21T21:58:49Z

The errors in the runs are from TLS issues, unrelated to my changes. Should I rebase my branch or something?

jalcine · 2019-10-25T22:10:43Z

hey @teamon; can you help me with this?

teamon · 2019-11-03T19:40:28Z

Yes, please start with rebase

jalcine · 2019-11-04T03:01:44Z

Rebased and I'm getting the same TLS errors.

teamon · 2019-11-04T08:57:07Z

True, I'll take a look what's going on there and I'll merge this PR once fixed.

teamon · 2019-11-05T23:08:57Z

The ssl issue seems to be resolved on master now.

I'd merge this right away, but I've noticed there is one comment left - https://github.com/teamon/tesla/pull/287/files#r270899880.

jalcine · 2019-11-22T21:09:07Z

I decided to forgo the regex and use string manipulation functions. I know this wasn't was prescribed originally but it works with the tests and follows the specification.

teamon · 2019-11-25T10:03:21Z

lib/tesla/middleware/decode_rels.ex

-    Regex.run(~r/\A<(.+)>; rel="(.+)"\z/, item, capture: :all_but_first)
-    |> Enum.reverse()
-    |> List.to_tuple()
+    [url, param] = String.split(item, ";")


Won't this break with rel="some;thing" ?

I can add a test to check.

teamon · 2020-04-07T10:44:05Z

@jalcine Hey, what's the status of this PR?

jalcine · 2020-05-15T07:54:44Z

Still working on this. Spent some time reading specs hoping I could close this ticket but it looks like it's expected of parser to understand the token form <https://URL> rel=token and quoted forms <https://URL> rel="token". It doesn't mention what kind of quote so I'll focus on the double quote form.

Spec reference: https://tools.ietf.org/html/rfc8288#section-3

jalcine · 2020-05-15T07:58:40Z

https://regex101.com/r/tdUd2o/1 is what I've been using to test.

jalcine · 2020-05-19T09:43:46Z

Bumping this @teamon

teamon reviewed Apr 11, 2019

View reviewed changes

teamon added the awaiting-feedback label Apr 24, 2019

teamon added the WIP label Sep 5, 2019

teamon mentioned this pull request Sep 5, 2019

Catch Empty Run Values #286

Closed

jalcine force-pushed the jalcine/check-regex-run-results branch from 707ee0f to 493f2f9 Compare November 4, 2019 02:57

jalcine force-pushed the jalcine/check-regex-run-results branch 2 times, most recently from 86bd577 to e414c7f Compare November 20, 2019 21:23

jalcine requested a review from teamon November 22, 2019 22:01

teamon reviewed Nov 25, 2019

View reviewed changes

jalcine added 4 commits May 15, 2020 00:37

Add test example of unquoted rel.

b8e14f6

A fix for the extra quoting of rel names.

4cf7821

decode_rels: Update w.r.t comments.

8065bb5

opt for using String functions.

a70e188

Fixed the case if multiple rel attributes came in.

dcacf91

jalcine force-pushed the jalcine/check-regex-run-results branch from d08c0b5 to dcacf91 Compare May 15, 2020 07:38

add failing case with embedded semi colon.

b6d645e

decode_rels: Update Regex to expectation.

560e767

jalcine requested a review from teamon May 20, 2020 08:32

teamon merged commit 593c717 into elixir-tesla:master May 27, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add test example of unquoted rel. #287

Add test example of unquoted rel. #287

jalcine commented Mar 27, 2019

jalcine commented Mar 27, 2019

codecov bot commented Mar 27, 2019

codecov bot commented Mar 27, 2019 •

edited

Loading

teamon Apr 1, 2019

jalcine Oct 21, 2019

teamon Apr 7, 2020

teamon Apr 1, 2019

teamon Apr 11, 2019

jalcine Nov 20, 2019

jalcine Nov 20, 2019

teamon Apr 7, 2020

teamon commented Apr 24, 2019

jalcine commented Apr 24, 2019

teamon commented May 31, 2019

teamon commented Jul 22, 2019

teamon commented Sep 5, 2019

jalcine commented Sep 13, 2019

jalcine commented Oct 21, 2019

jalcine commented Oct 25, 2019

teamon commented Nov 3, 2019

jalcine commented Nov 4, 2019

teamon commented Nov 4, 2019

teamon commented Nov 5, 2019

jalcine commented Nov 22, 2019

teamon Nov 25, 2019

jalcine Nov 26, 2019

teamon commented Apr 7, 2020

jalcine commented May 15, 2020

jalcine commented May 15, 2020

jalcine commented May 19, 2020

Add test example of unquoted rel. #287

Add test example of unquoted rel. #287

Conversation

jalcine commented Mar 27, 2019

jalcine commented Mar 27, 2019

codecov bot commented Mar 27, 2019

Codecov Report

codecov bot commented Mar 27, 2019 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

teamon commented Apr 24, 2019

jalcine commented Apr 24, 2019

teamon commented May 31, 2019

teamon commented Jul 22, 2019

teamon commented Sep 5, 2019

jalcine commented Sep 13, 2019

jalcine commented Oct 21, 2019

jalcine commented Oct 25, 2019

teamon commented Nov 3, 2019

jalcine commented Nov 4, 2019

teamon commented Nov 4, 2019

teamon commented Nov 5, 2019

jalcine commented Nov 22, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

teamon commented Apr 7, 2020

jalcine commented May 15, 2020

jalcine commented May 15, 2020

jalcine commented May 19, 2020

codecov bot commented Mar 27, 2019 •

edited

Loading