[saem-devel] [PATCH oaipmh V2] [views] switch to pyramid views

Denis Laxalde denis.laxalde at logilab.fr
Wed Feb 22 12:06:34 CET 2017


Philippe Pepiot a écrit :
> # HG changeset patch
> # User Philippe Pepiot <philippe.pepiot at logilab.fr>
> # Date 1487075346 -3600
> #      Tue Feb 14 13:29:06 2017 +0100
> # Node ID aed1492620d5bf43a0d7a38fa7ad8cb1b9951054
> # Parent  7c62d81ae3c3ff7d1215063ee7766eacae355d66
> # Available At https://hg.logilab.org/review/cubes/oaipmh
> #              hg pull https://hg.logilab.org/review/cubes/oaipmh -r aed1492620d5
> # Tested at https://jenkins.logilab.org/job/cubicweb-oaipmh/9/
> [views] switch to pyramid views
>
> Use pyramid views instead of cubicweb views.
>
> Use a single predicate to match verb and handle errors in errors(), if errors()
> don't return None verb_content() is not called.
>
> Fix some tests where metadataPrefix wasn't set explicitely, it was working
> before since req.form keep old values along requests.

This should go in a separate changeset.

> Depend on cubicweb[pyramid] >= 3.24.5
>
> Related to extranet #11855076
>
> diff --git a/__init__.py b/__init__.py
> --- a/__init__.py
> +++ b/__init__.py
> @@ -90,3 +90,7 @@ class ResumptionToken(object):
>          """Return string encoding state of the resumption token."""
>          if self:
>              return base64.urlsafe_b64encode(str(self))
> +
> +
> +def includeme(config):
> +    config.include('.views')
> diff --git a/__pkginfo__.py b/__pkginfo__.py
> --- a/__pkginfo__.py
> +++ b/__pkginfo__.py
> @@ -19,7 +19,7 @@ description = 'OAI-PMH server for CubicW
>  web = 'http://www.cubicweb.org/project/%s' % distname
>
>  __depends__ = {
> -    'cubicweb': '>= 3.22.1',
> +    'cubicweb[pyramid]': '>= 3.24.5',
>      'six': '>= 1.4.0',
>      'python-dateutil': None,
>      'isodate': None,
> diff --git a/cubicweb-oaipmh.spec b/cubicweb-oaipmh.spec
> --- a/cubicweb-oaipmh.spec
> +++ b/cubicweb-oaipmh.spec
> @@ -20,7 +20,7 @@ BuildArch:      noarch
>  BuildRoot:      %{_tmppath}/%{name}-%{version}-%{release}-buildroot
>
>  BuildRequires:  %{python} %{python}-setuptools
> -Requires:       cubicweb >= 3.22.1
> +Requires:       cubicweb >= 3.24.5
>  Requires:       %{python}-six >= 1.4.0
>  Requires:       python-isodate
>  Requires:       python-dateutil
> diff --git a/debian/control b/debian/control
> --- a/debian/control
> +++ b/debian/control
> @@ -11,7 +11,7 @@ X-Python-Version: >= 2.6
>  Package: cubicweb-oaipmh
>  Architecture: all
>  Depends:
> - cubicweb-common (>= 3.22.1),
> + python-cubicweb (>= 3.24.5),
>   python-six (>= 1.4.0),
>   python-isodate,
>   python-dateutil,
> diff --git a/entities.py b/entities.py
> --- a/entities.py
> +++ b/entities.py
> @@ -269,7 +269,11 @@ class OAIPMHRecordAdapter(EntityAdapter)
>              _, vid = self.metadata_formats[prefix]
>          except KeyError:
>              raise NoRecordsMatch('unsupported metadata prefix "{0}"'.format(prefix))
> -        data = self._cw.view(vid, w=None, rset=self.entity.as_rset())
> +        # XXX: use cw_request instead of self._cw which is a
> +        # cubicweb.pyramid.core.Connection and doesn't have list_form_param()
> +        # method used in view xmlitem
> +        cw = self._cw._session._pyramid_request.cw_request
> +        data = cw.view(vid, w=None, rset=self.entity.as_rset())

If you do something like
'request.cw_request.entity_from_eid(entity.eid)' on Pyramid-view side,
does it solve the problem?
At least until we avoid to call a view from this adapter.

>          if isinstance(data, unicode):
>              # Underlying view may be 'binary' or not.
>              data = data.encode('utf-8')
> diff --git a/test/data/pyramid.ini b/test/data/pyramid.ini
> new file mode 100644
> --- /dev/null
> +++ b/test/data/pyramid.ini
> @@ -0,0 +1,7 @@
> +[main]
> +# to get traceback properly in tests
> +cubicweb.bwcompat = no
> +# avoid noise in test output
> +cubicweb.session.secret = secret
> +cubicweb.auth.authtkt.session.secret = secret
> +cubicweb.auth.authtkt.persistent.secret = secret

Why do we need this? I guess only `cubicweb.bwcompat = no` is needed and
this should be set as a `settings` class attribute on test case class.

> diff --git a/test/test_oaipmh.py b/test/test_oaipmh.py
> --- a/test/test_oaipmh.py
> +++ b/test/test_oaipmh.py
> @@ -30,6 +30,7 @@ from six import text_type
>  from logilab.common import tempattr
>
>  from cubicweb.devtools.testlib import CubicWebTC
> +from cubicweb.pyramid.test import PyramidCWTest
>
>  from cubes.oaipmh import utcnow, MetadataFormat, ResumptionToken
>  from cubes.oaipmh.entities import OAIComponent, NoRecordsMatch
> @@ -261,7 +262,7 @@ def xmlpp(string):
>      print(etree.tostring(etree.fromstring(string), pretty_print=True))
>
>
> -class OAIPMHViewsTC(CubicWebTC, OAITestMixin):
> +class OAIPMHViewsTC(PyramidCWTest, OAITestMixin):

         settings = {'cubicweb.bwcompat': False}

>      _validate_xml = True
>      _debug_xml = True
> @@ -278,8 +279,7 @@ class OAIPMHViewsTC(CubicWebTC, OAITestM
>          xmlschema.assertValid(root)
>
>      def oai_request(self, req, **formparams):
> -        req.form.update(formparams)
> -        out = self.app_handle_request(req, 'oai')
> +        out = self.webapp.get('/oai', formparams).body
>          if self._validate_xml:
>              self.assertXmlValid(out, self.datapath('OAI-PMH.xsd'), debug=self._debug_xml)
>          return out
> @@ -423,10 +423,10 @@ class OAIPMHViewsTC(CubicWebTC, OAITestM
>
>          def check_request(expected_identifiers, token=None):
>              with self.admin_access.web_request() as req:
> +                kwargs = {'resumptionToken': token} if token is not None else {}
>                  result = self.oai_request(
>                      req, verb='ListIdentifiers', set='agent',
> -                    metadataPrefix='oai_dc',
> -                    resumptionToken=token)
> +                    metadataPrefix='oai_dc', **kwargs)
>                  # Ensure there are as many <identifier> tag than expected items.
>                  self.assertEqual(
>                      result.count('<identifier>'), len(expected_identifiers))
> @@ -491,6 +491,7 @@ class OAIPMHViewsTC(CubicWebTC, OAITestM
>              self.assertIn('<identifier>{0}</identifier>'.format(alice), result)
>              self.assertEqual(result.count('<identifier>'), 2)
>              result = self.oai_request(req, verb='ListIdentifiers',
> +                                      metadataPrefix='oai_dc',
>                                        set='agent:kind:person')
>              self.assertIn('<identifier>{0}</identifier>'.format(alice),
>                            result)
> diff --git a/tox.ini b/tox.ini
> --- a/tox.ini
> +++ b/tox.ini
> @@ -5,6 +5,7 @@ envlist = py27,flake8
>  sitepackages = true
>  deps =
>      pytest
> +    webtest
>  commands =
>    {envpython} -m pytest {posargs:{toxinidir}/test}
>
> diff --git a/views.py b/views.py
> --- a/views.py
> +++ b/views.py
> @@ -13,7 +13,7 @@
>  #
>  # You should have received a copy of the GNU Lesser General Public License along
>  # with this program. If not, see <http://www.gnu.org/licenses/>.
> -"""cubicweb-oaipmh views for OAI-PMH export
> +"""cubicweb-oaipmh pyramid views for OAI-PMH export
>
>  Set hierarchy specification
>  ---------------------------
> @@ -51,11 +51,8 @@ from lxml import etree
>  from lxml.builder import E, ElementMaker
>  import pytz
>
> -from logilab.common.registry import objectify_predicate
> -
> -from cubicweb.predicates import ExpectedValuePredicate, match_form_params
> -from cubicweb.view import View
> -from cubicweb.web.views import urlrewrite
> +from pyramid.response import Response
> +from pyramid.view import view_config
>
>  from cubes.oaipmh import utcnow, OAIError, ResumptionToken
>
> @@ -74,20 +71,6 @@ def utcparse(timestr):
>          return date.astimezone(pytz.utc)
>
>
> -class match_verb(ExpectedValuePredicate):
> -    """Predicate checking `verb` request form parameter presence and value.
> -
> -    Return 2 in case of match, for precedence over
> -    ``match_form_params('verb')``.
> -    """
> -
> -    def __call__(self, cls, req, rset=None, **kwargs):
> -        verb = req.form.get('verb')
> -        if verb is None:
> -            return 0
> -        return 2 * int(verb in self.expected)
> -
> -
>  def filter_none(mapping):
>      """Return a dict from `mapping` with None values filtered out."""
>      out = {}
> @@ -132,7 +115,7 @@ class OAIRequest(object):
>
>      @classmethod
>      def from_request(cls, baseurl, request):
> -        form = request.form
> +        form = request.params

rather get rid of the 'form' variable and use request.params directly

>          return cls(
>              baseurl,
>              setspec=form.get('set'),
> @@ -347,19 +330,10 @@ class OAIRecord(object):
>          return E.record(*elems)
>
>
> -class OAIPMHRewriter(urlrewrite.SimpleReqRewriter):
> -    rules = [('/oai', dict(vid='oai'))]
> -
> -
> -class OAIView(View):
> +class OAIView(object):
>      """Base class for any OAI view, subclasses should either implement
>      `errors` or `verb_content` methods.
>      """
> -    __regid__ = 'oai'
> -    __abstract__ = True
> -    templatable = False
> -    content_type = 'text/xml'

Aren't we loosing the Content-Type header on response?

> -    binary = True
>
>      @staticmethod
>      def verb_content():
> @@ -368,52 +342,52 @@ class OAIView(View):
>      @staticmethod
>      def errors():
>          """Return the errors of the OAI-PMH request."""
> -        return {}
> +        return
>
> -    def __init__(self, *args, **kwargs):
> -        super(OAIView, self).__init__(*args, **kwargs)
> +    def __init__(self, request):
> +        self.request = request
> +        self._cw = request.cw_cnx
> +        baseurl = request.cw_cnx.build_url('oai')

We should get rid of baseurl and use "request.route_url('oai', ...)"
(might be a separate patch).

>          self.oai_request = OAIRequest.from_request(
> -            self._cw.build_url('oai'), self._cw)
> +            baseurl, request)
>
> -    def call(self):
> +    def __call__(self):
>          encoding = self._cw.encoding
>          assert encoding == 'UTF-8', 'unexpected encoding {0}'.format(encoding)
> -        self.w('<?xml version="1.0" encoding="%s"?>\n' % encoding)
> +        content = '<?xml version="1.0" encoding="%s"?>\n' % encoding
>          oai_response = OAIResponse(self.oai_request)
>          # combine errors coming from view selection with those of request
>          # processing.
> -        errors = self.errors()
> +        errors = self.errors() or {}
> +        verb_content = self.verb_content() if not errors else None
>          errors.update(self.oai_request.errors)
> -        response_elem = oai_response.to_xml(self.verb_content(), errors=errors)
> -        self.w(etree.tostring(response_elem, encoding='utf-8'))
> +        response_elem = oai_response.to_xml(verb_content, errors=errors)
> +        content += etree.tostring(response_elem, encoding='utf-8')
> +        return Response(content)

This class might be better defined as a custom renderer which would both
handle response with and without errors using an exception control flow
rather than this clumsy `self.errors()` thing. (Future patch.)

>
>
> + at view_config(route_name='oai')
>  class OAIBaseView(OAIView):
> -    """Base view for OAI-PMH request with no "verb" specified.
> +    """Base view for OAI-PMH request with no or bad "verb" specified.

Rename the class (not any more a base view, but an error view).

>
>      `verb` request parameter is necessary in our implementation.
>      """
>
>      def errors(self):
> -        return {'badVerb': 'no verb specified'}
> +        verb = self.request.params.get('verb')
> +        if verb:
> +            return {'badVerb': 'illegal verb "{0}"'.format(self.oai_request.verb)}
> +        else:
> +            return {'badVerb': 'no verb specified'}
>
>
> -class OAIWithVerbView(OAIView):
> -    """Base view for OAI-PMH request with a "verb" specified.
> -
> -    This view generated an error as the implementation relies on explicit view
> -    to handle supported verbs.
> -    """
> -    __select__ = match_form_params('verb')
> + at view_config(route_name='oai', verb='Identify')
> +class OAIIdentifyView(OAIView):
> +    """View handling verb="Identify" requests."""
>
>      def errors(self):
> -        """Return the errors of the OAI-PMH request."""
> -        return {'badVerb': 'illegal verb "{0}"'.format(self.oai_request.verb)}
> -
> -
> -class OAIIdentifyView(OAIView):
> -    """View handling verb="Identify" requests."""
> -    __select__ = match_verb('Identify')
> +        if set(self.request.params) - set(['verb', 'vid']):
> +            return {'badArgument': 'Identify accepts no argument'}
>
>      def verb_content(self):
>          oai = self._cw.vreg['components'].select('oai', self._cw)
> @@ -435,51 +409,28 @@ class OAIIdentifyView(OAIView):
>          yield E('granularity', 'YYYY-MM-DDThh:mm:ssZ')
>
>
> - at objectify_predicate
> -def no_params_in_form(cls, req, **kwargs):
> -    """Return 1 if req.form only has "verb" (and "vid") parameters."""
> -    extra = set(req.form) - set(['verb', 'vid'])
> -    return 0 if extra else 1
> -
> + at view_config(route_name='oai', verb='ListMetadataFormats')
> +class OAIListMetadatFormatsByIdentifierView(OAIView):
> +    """View handling verb="ListMetadataFormats" requests."""
>
> -class OAIIdentifyBadArgumentsView(OAIView):
> -    """View handling verb="Identify" requests but with bad arguments"""
> -    __select__ = match_verb('Identify') & ~no_params_in_form()
> -
> -    def errors(self):
> -        return {
> -            'badArgument': 'Identify accepts no argument'}
> +    def verb_content(self):
> +        identifier = self.request.params.get('identifier')
> +        if identifier:
> +            rset = self.oai_request.rset_from_identifier(self._cw)

If you pass self.request.cw_request instead of self._cw, you might be
able to drop the hack in entities.py above.

> +            if not rset:
> +                raise IdDoesNotExist(self.oai_request.identifier)
> +            for record in oai_records(rset):
> +                for fmt in record.metadata_formats():
> +                    yield fmt
> +        else:
> +            oai = self._cw.vreg['components'].select('oai', self._cw)
> +            for prefix, fmt in oai.metadata_formats():
> +                yield xml_metadataformat(prefix, fmt)
>

[snip]

The following looks ok as long as tests keep passing.



More information about the saem-devel mailing list