Set the correct media type for login response and parse the Accept header

[marmoure]

This PR adds parsing for Accept header and introduces local:accept-json and local:preferred-media-type, it build on top of #800 to fix #253.
These functions allow more robust and future proof way for getting the appropriate header either the preferred one or a specific one.

Also when trying to set the correct headers for the login response using

util:declare-option("output:method", "json"),
util:declare-option("output:medita-type", "application/json"),

I encountered a bug in the RESTServer in writeResultJSON which ignored the output:medita-type option, I added evolvedbinary/elemental#212 for elemental and the same should be done for eXist-db.

In the mean time

response:set-header("Content-Type", "application/json; charset=UTF-8"),

is being used as workaround until this bug is resolved.

[duncdrum]

Thanks for the PR @marmoure. The Xqdoc is not well-formed.

We don't normally link elemental issues. Feel free to keep it in alongside an exist issue. But both should be properly formatted.

Please add a test. Did you test this against exist? Which version?

[marmoure]

Thanks for the @duncdrum I fixed the xqdoc issue, this was tested on eXist-db 6.4.1.

[duncdrum]

@marmoure could you add a test for the request to the testsuite, so we see how this plays in against different backends.

[adamretter]

@duncdrum I believe that @marmoure has mentioned in the past that the eXide test suite is flaky, but it doesn't look like that has been addressed since then. Therefore we don't have much confidence in the eXide test suite.

We sent this code as a courtesy on behalf of one of your users of eXide. I am very reluctant to have @marmoure invest more time in this, because (a) we already have a fix for Elemental, and (b) previous attempts where we invested significant time into improving the eXist-db Apps in collaboration with you did not go well (i.e. eXist-db/monex#298).

Can you provide any sort of assurance that if we invest further time and that if @marmoure provides a test for this, then you will merge it?

Thanks.

[duncdrum]

@adamretter our review policy hasn't changed. You can see #799 for reference. Comparing this here to the full rewrite of monex is comparing apples to oranges.

What I can assure you is that I won't merge without a test. If you wish to invest more time into this version of eXide in light of #794 is up to you.

[adamretter]

our review policy hasn't changed

What I can assure you is that I won't merge without a test

@duncdrum Er... but... you just merged the following PR from another external contributor without a test - https://github.com/eXist-db/eXide/pull/799/changes

[marmoure]

Hi @duncdrum I m bit confused here, I m seeing a CI fail and when I opened the screenshots I m confident that's not the code on this branch.
What is being tested in this case? and how can I reproduce this testcase locally?

[duncdrum]

@adamretter yes because the existing test suite caught a regression. As is visible in the comments.

@marmoure I m confused the screenshot is against latest major exide / exist v7. The issue was opened against previous / exist 6.

What's the target? Rebase and fix against latest? Or still the original target.

[marmoure]

I m confused the screenshot is against latest major exide / exist v7. The issue was opened against previous / exist 6.

@duncdrum I m confused as well, I didn't expect to see screenshot from eXide V4 as this PR is quite old and still has the code for 3.5.5, the CI seems to be running the tests against V4.

Rebase and fix against latest

Yes that would work, and I will do that, but I wanted to bring this matter to your attention first as I find it quite strange, if we are running the tests against the Latest exist and eXist-db does that mean only the tests are running? what about the code changes?

[duncdrum]

@marmoure the code changes in this branch without rebase are v3. You should see that when you run the tests locally.

What ci runs against depends on the ci script in this branch, and how exide is installed there.

[marmoure]

Hi @duncdrum I rebased, and updated the test to work with the new V4 api.

[duncdrum]

Thank you @marmoure . The test is certainly helpful, I'm not sure if the controller change is still necessary in v4.

[line-o]

@duncdrum the authentication will be removed entirely from the controller see #814

[marmoure]

Thanks @duncdrum, What should I do in this case?