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

Improve content negociation middlewares #150

Merged
merged 31 commits into from May 13, 2016

Conversation

Projects
None yet
2 participants
@arteymix
Member

arteymix commented Jan 12, 2016

In short, it handles fuzzy matching and set the appropriate header in the response.

using Valum.ContentNegotiation;

app.get ("", accept ("text/html", (req, res) => {
    res.body.write_all ("<!DOCTYPE html><html></html>", null);
})).then (accept ("text/xhtml", (req, res) => {
    res.body.write_all ("...");
}, NegociateFlags.FINAL));

The NegociateFlags.FINAL indicate that this is the last option and a 406 Not Acceptable should be raised on failure.

It needs some tests before being merged.

Improve content negociation middlewares
Fuzzy matching is provided by submitting a custom compare function to
'negociate'.

In short,

 - 'accept' handle '*/*' and 'type/*' patterns.
 - 'accept_charset', 'accept_encoding' and 'accept_language' handle '*'

Set the corresponding response header when a negociation succeeds.

If a charset is supplied to 'accept', it is negociated with
'accept_charset'.

Provide 'NegociateFlags.FINAL' to raise a 'ClientError.NOT_ACCEPTABLE'
if the negociation fails.

Provide minimal tests for 'negociate' and 'accept'.

@arteymix arteymix added the feature label Jan 12, 2016

@arteymix arteymix self-assigned this Jan 12, 2016

@arteymix arteymix added this to the 0.3.0 milestone Jan 12, 2016

Provide content negotiation in the 'Valum.ContentNegotiation' namespace
Fix a bad typo as well, I got messed up with my french.
@arteymix

This comment has been minimized.

Show comment
Hide comment
@arteymix

arteymix Jan 12, 2016

Member

It would be great to have something that allow us to build sequences and connect the last next to the keep routing after the sequence.

Something like:

app.get ("", sequence(
    accept ("text/xml", produce_xml),
    accept ("text/html", produce_html, NegociateFlags.FINAL)
));

That could get rid of then.

Member

arteymix commented Jan 12, 2016

It would be great to have something that allow us to build sequences and connect the last next to the keep routing after the sequence.

Something like:

app.get ("", sequence(
    accept ("text/xml", produce_xml),
    accept ("text/html", produce_html, NegociateFlags.FINAL)
));

That could get rid of then.

@arteymix

This comment has been minimized.

Show comment
Hide comment
@arteymix

arteymix Feb 5, 2016

Member

It would be great to handle quality values in some way.

Member

arteymix commented Feb 5, 2016

It would be great to handle quality values in some way.

@arteymix

This comment has been minimized.

Show comment
Hide comment
@arteymix

arteymix Feb 5, 2016

Member

To handle quality values properly, we could provide multiple choices to negotiate and let it forward the best possible:

accept ({"text/html", "text/html+xml"}, (req, res, next, stack, content_type) {
    // ...
}};
Member

arteymix commented Feb 5, 2016

To handle quality values properly, we could provide multiple choices to negotiate and let it forward the best possible:

accept ({"text/html", "text/html+xml"}, (req, res, next, stack, content_type) {
    // ...
}};
@arteymix

This comment has been minimized.

Show comment
Hide comment
@arteymix

arteymix Feb 5, 2016

Member

The tests work, but it produces a SEGFAULT when running the application :(

Member

arteymix commented Feb 5, 2016

The tests work, but it produces a SEGFAULT when running the application :(

@arteymix

This comment has been minimized.

Show comment
Hide comment
@arteymix

arteymix Feb 5, 2016

Member

Okay, everything is correct now!

Since we handle multiple expectations, the default behaviour is to raise a 406 Not Acceptable, which can be overwritten by setting the NegotiateFlags.NEXT.

No expectations always raise a 406 Not Acceptable and a missing header forwards the first expectation.

We need some tests for the edge cases and I have some documentation already ;)

Member

arteymix commented Feb 5, 2016

Okay, everything is correct now!

Since we handle multiple expectations, the default behaviour is to raise a 406 Not Acceptable, which can be overwritten by setting the NegotiateFlags.NEXT.

No expectations always raise a 406 Not Acceptable and a missing header forwards the first expectation.

We need some tests for the edge cases and I have some documentation already ;)

arteymix added some commits Feb 5, 2016

Negotiate multiple expectations
Provide a list of expecations to 'negotiate', which will forward the one
with the highest quality value.

Update docs and examples.
Raise a '406 Not Acceptable' by default in 'negotiate'
As we can handle quality values, it's not useful to call 'next' by
default. Instead, a '406 Not Acceptable' is raised if none of the
expectations can be satisfied.

Replace 'NegotiateFlags.FINAL' by 'NegotiateFlags.NEXT' to call 'next'
instead of raising a '406 Not Acceptable' status.

Provide a 'forward' function to serve as a default value. It let
end-user write the following:

    app.use (accept ({"application/json"}));

Update tests to take these changes into consideration.
If no header is present, assume that anything is acceptable
Systematically raise a '406 Not Acceptable' if the expectations array is
empty.

Fix the SEGFAULT when the application is running, it looks like a Vala
memory bug.

Fix the example by correcting a typo and setting the
'NegotiateFlags.NEXT' flag.
Convert the charset from the old one in 'accept_charset'
If no charset were explicitly set, it is assumed to be 'utf-8'. If it's
unchanged, don't do anything.
Tests for 'negotiate' and 'accept'
Use an explicit reference for 'forward'.
Perform case-insensitive comparison
Replace the comparison function by an equality ('EqualFunc<string>') and
use 'Soup.str_case_equal'.

Lowercase the encoding in 'accept_encoding' in order to determine what
converter is being applied.
@arteymix

This comment has been minimized.

Show comment
Hide comment
@arteymix

arteymix Feb 5, 2016

Member

The tests have to cover the use of mixed character case, because most of the specification is case-insensitive.

Member

arteymix commented Feb 5, 2016

The tests have to cover the use of mixed character case, because most of the specification is case-insensitive.

@arteymix

This comment has been minimized.

Show comment
Hide comment
@arteymix

arteymix Feb 25, 2016

Member

The merge will have to deal with a couple of breaking changes:

  • return the result of forward
  • stack is a context (not used anyway)
Member

arteymix commented Feb 25, 2016

The merge will have to deal with a couple of breaking changes:

  • return the result of forward
  • stack is a context (not used anyway)
@arteymix

This comment has been minimized.

Show comment
Hide comment
@arteymix

arteymix Feb 29, 2016

Member

Just picked the codecov commit, so that we could see what parts are missing. It should end-up here: https://codecov.io/github/valum-framework/valum?pr=150

Member

arteymix commented Feb 29, 2016

Just picked the codecov commit, so that we could see what parts are missing. It should end-up here: https://codecov.io/github/valum-framework/valum?pr=150

Improve 'accept_charset' middleware
Assume the default charset to be 'iso-8859-1' as specified in
'HTTP/1.1'.

Only apply conversion on 'text/*' and use a case-insensitive comparison
for the charset. It's not a convenient to transform non-text data,
especially since it could break binary codings.
@arteymix

This comment has been minimized.

Show comment
Hide comment
@arteymix

arteymix May 10, 2016

Member

It would be nice to use a quality list for the expectations and select the best match by taking the highest qvalue product. This is what Apache uses.

app.get ("/", accept ("text/json; q=1, text/xml; q=0.1", (req, res, next, ctx, expectation) => {
    // produce content according to 'expectation'
}));
Member

arteymix commented May 10, 2016

It would be nice to use a quality list for the expectations and select the best match by taking the highest qvalue product. This is what Apache uses.

app.get ("/", accept ("text/json; q=1, text/xml; q=0.1", (req, res, next, ctx, expectation) => {
    // produce content according to 'expectation'
}));

arteymix added some commits May 10, 2016

negotiation: Remove 'forward' callback and its default usage
The typical use case for this middleware is to explicitly pass a
callback, so it's not useful to invoke 'next' as a default.
negotiation: Negotiate the best quality and preference product
The expectations list can now contain a 'q' parameter which specify the
quality of the specified item.

It is negotiated such that the product of the quality and preference is
maximized.
negotiation: Remove charset conversion
Choosing when to apply charset conversion is highly depending on the
content type and it would be really difficult to perform that correctly.

Instead, just let the end-user decide how charset should be applied.
Merge branch 'negociation-middlewares' into 'master'
Update the code to the latest change in the 'master' trunk.
negotiation: Remove 'NegotiateFlags.NEXT'
The same result can be performed with a status handler.

Remove the 'NegotiateFlags' as well.
negotiation: Update the documentation
Cover how preference and quality are processed.
@arteymix

This comment has been minimized.

Show comment
Hide comment
@arteymix

arteymix May 12, 2016

Member

I merged the changes from the 0.3 so it should be fast-forward into the trunk.

  • support vendor prefix for accept_encoding
  • documentation update
  • fix for qvalue extraction
  • remove NegotiateFlags.NEXT in favour of a status-handler based approach

Some tests are missing and it should be ready to merge!

Member

arteymix commented May 12, 2016

I merged the changes from the 0.3 so it should be fast-forward into the trunk.

  • support vendor prefix for accept_encoding
  • documentation update
  • fix for qvalue extraction
  • remove NegotiateFlags.NEXT in favour of a status-handler based approach

Some tests are missing and it should be ready to merge!

arteymix added some commits May 12, 2016

negotiation: Fix qvalue extraction end offset
For trailing item, it would slice until '-1', which would strip the last
digit of the qvalue.
negotiation: Make global variant acceptable for regional preference
For the 'accept_language' middleware, make global variant (eg. 'en')
acceptable for regional preference (eg. 'en-GB'). This invert the
precedent behavior which was not appropriate.
@codecov-io

This comment has been minimized.

Show comment
Hide comment
@codecov-io

codecov-io May 13, 2016

Current coverage is 63.28%

Merging #150 into master will increase coverage by +2.11%

@@             master       #150   diff @@
==========================================
  Files            28         28          
  Lines          1025       1084    +59   
  Methods           0          0          
  Messages          0          0          
  Branches          0          0          
==========================================
+ Hits            627        686    +59   
  Misses          398        398          
  Partials          0          0          
  1. File ...i/vsgi-response.vala was modified. more
    • Misses -2
    • Partials 0
    • Hits +2

Powered by Codecov. Last updated by b7870a2...42c1ab4

codecov-io commented May 13, 2016

Current coverage is 63.28%

Merging #150 into master will increase coverage by +2.11%

@@             master       #150   diff @@
==========================================
  Files            28         28          
  Lines          1025       1084    +59   
  Methods           0          0          
  Messages          0          0          
  Branches          0          0          
==========================================
+ Hits            627        686    +59   
  Misses          398        398          
  Partials          0          0          
  1. File ...i/vsgi-response.vala was modified. more
    • Misses -2
    • Partials 0
    • Hits +2

Powered by Codecov. Last updated by b7870a2...42c1ab4

@arteymix arteymix merged commit 426368e into master May 13, 2016

3 checks passed

codecov/project 63.38% (+2.20%) compared to b7870a2
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details

@arteymix arteymix deleted the negociation-middlewares branch May 13, 2016

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