Onderscheid op accept header toelaten

claeyswo commented 1 year ago

Nu ziet een regel er zo uit:

- match: '^/archeologie/metaaldetectievondstmeldingen/(?P<id>\d+)$'
      redirect: 'https://@@archeologie.url@@/metaaldetectievondstmeldingen/beheer/#/{id}'

We zouden iets zoals dit willen ondersteunen:

- match: '^/archeologie/metaaldetectievondstmeldingen/(?P<id>\d+)$'
      redirect: 
        - text/html: 'https://@@archeologie.url@@/metaaldetectievondstmeldingen/beheer/#/{id}'
        - application/json: 'https://@@archeologie.url@@/metaaldetectievondstmeldingen/metaaldetectievondsten/{id}'

Misschien niet meer dan hier extra check op u[redirect] is list => filteren op u[redirect][request.accept] of zoets?

goessebr commented 1 year ago

Zelfde fix is nodig voor toevalsvondsten (cfr. https://github.com/OnroerendErfgoed/inventaris/issues/5711#issuecomment-1419123095 en https://github.com/OnroerendErfgoed/inventaris/issues/5710#issuecomment-1419119400)

Indien JSON dan willen we naar https://test-loket.onroerenderfgoed.be/archeologie/vondstmeldingen/toevalsvondsten/545 (huidige standaard gedrag), indien HTML dan willen we naar /beheer (https://test-loket.onroerenderfgoed.be/archeologie/vondstmeldingen/toevalsvondsten/beheer) met het juiste ID geopend

koenedaele commented 1 year ago

Wat als er een mime-type gevraagd wordt dat niet gedefinieerd is?

koenedaele commented 1 year ago

Lijkt me een implementatie van 4.3 in https://www.w3.org/TR/cooluris/ te zijn.

claeyswo commented 1 year ago

Wat als er een mime-type gevraagd wordt dat niet gedefinieerd is?

altijd default voorzien
eerste uit de lijst
404
iets anders?

goessebr commented 1 year ago

Voorstel in pseudo code:

Indien type(redirect) == string: we aanvaarden alle Accept headers en vertalen naar de opgegeven waarde (huidig gedrag)
Indien type(redirect) == list:
- Indien opgevraagde type niet aanwezig als key in de configlist: 406 Not Acceptable (https://developer.mozilla.org/en-US/docs/Web/HTTP/Status/406).
- Er is ook een key "default" of iets gelijkaardig mogelijk, zodat we niet alle keys moeten opgeven als we slechts voor 1 mime-type een ander gedrag willen, maar alle andere mime-types naar dezelfde URL willen doorverwijzen. Dus indien "default" aanwezig in de lijst, treed er nooit een 406 op.

koenedaele commented 1 year ago

Akkoord met voorstel van Bram (met de default optie). Was even vergeten dat er een HTTP 406 was. Mooie oplossing!

Wim-De-Clercq commented 1 year ago

Mag ik nog een kleine syntax change in de yaml voorstellen:

- match: '^/archeologie/metaaldetectievondstmeldingen/(?P<id>\d+)$'
  redirect: 
    text/html: 'https://@@archeologie.url@@/metaaldetectievondstmeldingen/beheer/#/{id}'
    application/json: 'https://@@archeologie.url@@/metaaldetectievondstmeldingen/metaaldetectievondsten/{id}'

Dan zit onder redirect een dict ipv list met dicts.

redirect = {
  "text/html": "redirecturl",
  "application/json": "redirecturl",
}

vs

redirect = [
  {"text/html": "redirecturl"},
  {"application/json": "redirecturl"},
]

Dat eerste lijkt mij logischer, en je kan dan ook geen dubbele mimetypes hebben als bonus.

Als de request geen accept header definieert (en dus eigenlijk zegt alles te accepteren). Wil ik dan random het eerste nemen, of wil ik manueel op zoek gaan of text/html bestaat eerst?

goessebr commented 1 year ago

Als de request geen accept header definieert (en dus eigenlijk zegt alles te accepteren). Wil ik dan random het eerste nemen, of wil ik manueel op zoek gaan of text/html bestaat eerst?

Indien er een default key is voorzien -> redirect naar de waarde die daar is opgegeven. Indien geen default key is voorzien zou ik een 406 opgegeven in dit geval.

Wim-De-Clercq commented 1 year ago

Zou dat niet gaan overlappen met browser calls? Browsers sturen altijd */* op. En ik vrees dat dat ook een default zou zijn voor pyramid accept header... kzal effe zien nog..

Wim-De-Clercq commented 1 year ago

Ja, dat zou issues geven denk ik. Omdat voor pyramid geen header alles matcht.

alleja, ik zou wel kunnen kijken naar de class van die header, als die AcceptNoHeader is ...

Wim-De-Clercq commented 1 year ago

Na daily: Ik begreep bram zijn opmerking verkeerd. Hij bedoelde dat er in de yaml ergens een "default" key zou zijn. Dat is wel mogelijk.

goessebr commented 1 year ago

https://test-id.erfgoed.net/archeologie/metaaldetectievondstmeldingen/716 geeft 500 fout

[2023-02-17 11:50:05 +0100] [722001] [ERROR] Error handling request /archeologie/metaaldetectievondstmeldingen/716
Traceback (most recent call last):
  File "/var/projects/joeri/venv/lib/python3.8/site-packages/pyramid/tweens.py", line 13, in _error_handler
    response = request.invoke_exception_view(exc_info)
  File "/var/projects/joeri/venv/lib/python3.8/site-packages/pyramid/view.py", line 795, in invoke_exception_view
    raise HTTPNotFound
pyramid.httpexceptions.HTTPNotFound: The resource could not be found.

During handling of the above exception, another exception occurred:

Traceback (most recent call last):
  File "/var/projects/joeri/venv/lib/python3.8/site-packages/gunicorn/workers/gthread.py", line 271, in handle
    keepalive = self.handle_request(req, conn)
  File "/var/projects/joeri/venv/lib/python3.8/site-packages/gunicorn/workers/gthread.py", line 323, in handle_request
    respiter = self.wsgi(environ, resp.start_response)
  File "/var/projects/joeri/venv/lib/python3.8/site-packages/wsgicoers.py", line 212, in __call__
    return self.application(environ, custom_start_response)
  File "/var/projects/joeri/venv/lib/python3.8/site-packages/pyramid/router.py", line 270, in __call__
    response = self.execution_policy(environ, self)
  File "/var/projects/joeri/venv/lib/python3.8/site-packages/pyramid/router.py", line 279, in default_execution_policy
    return request.invoke_exception_view(reraise=True)
  File "/var/projects/joeri/venv/lib/python3.8/site-packages/pyramid/view.py", line 794, in invoke_exception_view
    reraise_(*exc_info)
  File "/var/projects/joeri/venv/lib/python3.8/site-packages/pyramid/compat.py", line 179, in reraise
    raise value
  File "/var/projects/joeri/venv/lib/python3.8/site-packages/pyramid/router.py", line 277, in default_execution_policy
    return router.invoke_request(request)
  File "/var/projects/joeri/venv/lib/python3.8/site-packages/pyramid/router.py", line 249, in invoke_request
    response = handle_request(request)
  File "/var/projects/joeri/venv/lib/python3.8/site-packages/pyramid/tweens.py", line 43, in excview_tween
    response = _error_handler(request, exc)
  File "/var/projects/joeri/venv/lib/python3.8/site-packages/pyramid/tweens.py", line 17, in _error_handler
    reraise(*exc_info)
  File "/var/projects/joeri/venv/lib/python3.8/site-packages/pyramid/compat.py", line 179, in reraise
    raise value
  File "/var/projects/joeri/venv/lib/python3.8/site-packages/pyramid/tweens.py", line 41, in excview_tween
    response = handler(request)
  File "/var/projects/joeri/venv/lib/python3.8/site-packages/pyramid/router.py", line 147, in handle_request
    response = _call_view(
  File "/var/projects/joeri/venv/lib/python3.8/site-packages/pyramid/view.py", line 683, in _call_view
    response = view_callable(context, request)
  File "/var/projects/joeri/venv/lib/python3.8/site-packages/pyramid/config/views.py", line 169, in __call__
    return view(context, request)
  File "/var/projects/joeri/venv/lib/python3.8/site-packages/pyramid/config/views.py", line 188, in attr_view
    return view(context, request)
  File "/var/projects/joeri/venv/lib/python3.8/site-packages/pyramid/config/views.py", line 214, in predicate_wrapper
    return view(context, request)
  File "/var/projects/joeri/venv/lib/python3.8/site-packages/pyramid/viewderivers.py", line 401, in viewresult_to_response
    result = view(context, request)
  File "/var/projects/joeri/venv/lib/python3.8/site-packages/pyramid/viewderivers.py", line 144, in _requestonly_view
    response = view(request)
  File "/var/projects/joeri/venv/lib/python3.8/site-packages/urihandler/views.py", line 13, in redirect
    redirect = request.uri_handler.handle(uri, request)
  File "/var/projects/joeri/venv/lib/python3.8/site-packages/urihandler/handler.py", line 33, in handle
    redirect = u["redirect"].format(**m.groupdict())
AttributeError: 'dict' object has no attribute 'format'

Dit is de yaml

    - match: '^/archeologie/metaaldetectievondstmeldingen/(?P<id>\d+)$'
      redirect:
        text/html: 'https://@@archeologie.url@@/metaaldetectievondstmeldingen/beheer/#/{id}'
        application/json: 'https://@@archeologie.url@@/metaaldetectievondstmeldingen/metaaldetectievondsten/{id}'

Wim-De-Clercq commented 1 year ago

Ik denk dat de urihandler package nog niet goed geupdate is als ik de lijn nummers zo zie.

goessebr commented 1 year ago

hm, ok. Ik bekijk het

goessebr commented 1 year ago

Het probleem was dat de versie van de urihandler package niet veranderd is op develop waardoor het package niet werd geupdated

Na een pip uninstall urihandler en nieuwe install was bovenstaande probleem voorbij

Nog 1 ander probleem

Dit is mijn config

    - match: '^/archeologie/metaaldetectievondstmeldingen/(?P<id>\d+)$'
      redirect:
        text/html: 'https://@@archeologie.url@@/metaaldetectievondstmeldingen/beheer/#/{id}'
        application/json: 'https://@@archeologie.url@@/metaaldetectievondstmeldingen/metaaldetectievondsten/{id}'
        default: 'https://@@inventaris.url@@/erfgoedobjecten/{id}'

Ik krijg een 406 voor https://dev-id.erfgoed.net/archeologie/metaaldetectievondstmeldingen/1 en Accept: application/xml, ik verwacht de erfgoedobjecten url

Wim-De-Clercq commented 1 year ago

Ah, dat heb ik fout begrepen dan. Ik had in mijn hoofd dat default enkel was voor het geval van dat er geen accept header aanwezig was.

goessebr commented 1 year ago

Ah, dat heb ik fout begrepen dan. Ik had in mijn hoofd dat default enkel was voor het geval van dat er geen accept header aanwezig was.

ah nee, default is de opvangbak van alle headers. Als alle gedefinieerde keys zijn gecontroleerd en we hebben geen match met onze header, dan is dit de redirect voor alles. Eerst moeten dus wel eerst alle keys gecontroleerd hebben want met voorkeur matchen we op het juiste type.

OnroerendErfgoed / urihandler

Onderscheid op accept header toelaten #86