Edge Case in ServerDrivenNegotiation with Accept-Language

[fibsifan]

Hello,

I've found a behaviour in ServerDrivenNegotiation that was unexpected to me. I'm unsure how to correctly address it though.

I created a PR #4738 to illustrate the Idea. When requesting with an Accept-Language Header, the order of the available options influences the outcome, if the options contain a mixture of languages with and without country variant. The disabled test behaves not as I would have expected.

I tried to understand RFC 2295 and RFC 2296 and figure out whether it provides insight in what the expected behaviour would be, but it looks like it doesn't - or I didn't read carefully enough, it's a lot of text.

But maybe someone here can help me understand whether I'm wrong or this behaviour is a bug?

[jamezp]

Hello @fibsifan and sorry for the delay. I do think this looks like a bug to me. I've got a fix locally that seems to fix the test as well. The only concern I'd have is this hasn't changed in possibly forever :) That said, I'd find it hard to argue when provided de-DE and asking for de-DE why that wouldn't win.

[jamezp]

Yeah. My initial fix was definitely not enough. I've got some tests and changes locally that hopefully I can have some better insight on this week.

[fibsifan]

I've managed to create a little jest test to see how node.js does it - looks like the behaviour is the same, regardless of the order of available languages.

Negotiator = require("negotiator");

describe("Language negotiation", () => {
    describe("specific first", () => {
        const availableLanguages = ["de-AT", "de-DE", "de", "en-US", "en"];
        requestLanguages(availableLanguages);
    });

    describe("generic first", () => {
        const availableLanguages = ["de", "de-AT", "de-DE", "en", "en-US"];
        requestLanguages(availableLanguages);
    });
})

function requestLanguages(availableLanguages) {
    test("request available specific language", () => {
        expect(accept("de-DE", availableLanguages)).toEqual("de-DE");
    });
    test("request unavailable specific language", () => {
        expect(accept("de-CH", availableLanguages)).toEqual("de");
    });
    test("request generic language", () => {
        expect(accept("de", availableLanguages)).toEqual("de");
    });
}

const accept = function (lang, available) {
    const req = {
        headers: {
            "accept-language": lang
        }
    }

    const negotiation = Negotiator(req);
    return negotiation.language(available) || "";
}

[jamezp]

Thank you! That would be my expectation as well. My current test that fails looks like:

@Test
public void mostSpecificVariant() {
    final List<Variant> available = List.of(
            new Variant(null, "en", "", null),
            new Variant(null, "en", "US", null),
            new Variant(null, "en", "GB", null),
            new Variant(null, "ja", "", null),
            new Variant(null, "ja", "JP", null),
            new Variant(null, "de", "", null),
            new Variant(null, "de", "CH", null),
            new Variant(null, "de", "DE", null));
    final ServerDrivenNegotiation negotiation = new ServerDrivenNegotiation();

    negotiation.setAcceptLanguageHeaders(List.of("de-DE", "de"));
    Assertions.assertEquals(Locale.forLanguageTag("de-DE"), negotiation.getBestMatch(available).getLanguage(),
            () -> String.format("Failed to match de-DE in %s", available));

    negotiation.setAcceptLanguageHeaders(List.of("de-LU"));
    Assertions.assertEquals(Locale.forLanguageTag("de"), negotiation.getBestMatch(available).getLanguage(),
            () -> String.format("Failed to match de-LU in %s", available));

    negotiation.setAcceptLanguageHeaders(List.of("de-CH"));
    Assertions.assertEquals(Locale.forLanguageTag("de-CH"), negotiation.getBestMatch(available).getLanguage(),
            () -> String.format("Failed to match de-CH in %s", available));

    negotiation.setAcceptLanguageHeaders(List.of("en-GB;q=0.7", "en"));
    Assertions.assertEquals(Locale.forLanguageTag("en"), negotiation.getBestMatch(available).getLanguage(),
            () -> String.format("Failed to match en with a quality level of 0.7 in %s", available));

    negotiation.setAcceptLanguageHeaders(List.of("en-GB", "en", "en-US"));
    Assertions.assertEquals(Locale.forLanguageTag("en-GB"), negotiation.getBestMatch(available).getLanguage(),
            () -> String.format("Failed to match en-GB in %s", available));

    negotiation.setAcceptLanguageHeaders(List.of("en-GB", "en"));
    Assertions.assertEquals(Locale.forLanguageTag("en-US"), negotiation.getBestMatch(available).getLanguage(),
            () -> String.format("Failed to match en in %s", available));
}

[fibsifan]

That looks good - except I don't agree with your last case. I think the expected result would be en-GB right?

[jamezp]

Well, I actually would expect it to be "en", but I can see the test is definitely wrong :) IoW the goal was to give it options "en-GB" and "en", but provide "en-US". I would expect that to return "en" as it's closer I think.

[jamezp]

I've filed RESTEASY-3675 for this issue. Nice find!

[michpetrov]

Given the available variants of [de, de-DE, de-AT] I'd expect the tests to run like this:

Accept-Language	result
de-DE	de-DE
de-CH	de
de	de-DE
de, de-AT	de-AT

For the last two specifically, de is the fallback variant and should not be served if de is accepted and specific variants are available. Accept-Language: de means "I accept any variant of German" and specific should have precedence over generic.
I would argue de, de-AT is nonsensical because the list is in order of priority and should be treated as de-AT, de.

This is in accordance with RFC4647, if I'm reading it correctly.

I would personally argue that de-CH should be treated as a shorthand for de-CH, de (in which case the second row would return de-DE) but I can see this might cause issues.

[fibsifan]

Thank you for bringing up RFC 4647.

I just checked: The RFC 2296 which ServerDrivenNegotiation refers to, is historical if I'm reading that correctly. So getting a "more correct" behaviour should indeed focus on newer best practices.

Apparently I read RFC4647 slightly different then you. I think in our case we want a Lookup (not filtering). Section 3.4 states

Lookup is used to select the single language tag that best matches
the language priority list for a given request. When performing
lookup, each language range in the language priority list is
considered in turn, according to priority. By contrast with
filtering, each language range represents the most specific tag that
is an acceptable match. The first matching tag found, according to
the user's priority, is considered the closest match and is the item
returned. For example, if the language range is "de-ch", a lookup
operation can produce content with the tags "de" or "de-CH" but never
content with the tag "de-CH-1996". If no language tag matches the
request, the "default" value is returned.

I agree that a request for de, de-AT would not make too much sense from a user perspective, but from reading the lookup specification it looks like the system still shouldn't return de-DE (unless that is the systems default). That is because de-DE neither matches the language rangede nor the language range de-AT in lookup mode.

[jamezp]

It took longer than expected, but this is now resolved see https://redhat.atlassian.net/browse/RESTEASY-3675. This fix will be in RESTEasy 6.2.17.Final and 7.0.3.Final.