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!