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

Philippe Pepiot philippe.pepiot at logilab.fr
Wed Feb 22 16:32:22 CET 2017


On 02/22/2017 12:06 PM, Denis Laxalde wrote:
> 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.
>

Sent a V2 with requested changes.

I changed 'self._cw = request.cw_cnx' to 'self._cw = request.cw_request' 
in OAIView.__init__ and it worked without needing this.

With some side effect like 'http://testing.fr/cubicweb' became 
'https://localhost.local/'...


>>          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.
>

(Added them in settings in V2 patch). Secrets are needed to avoid "!! 
WARNING !! WARNING !! You should STOP THIS INSTANCE NOW" in test output 
(stderr). I guess this should be fixed in cubicweb too.

>> 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
>

Ok done.

>>          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?
>

Good catch, added a check for this in tests

>> -    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).

Ok will be done in a followup

>
>>          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.)
>

Agreed, will be done in a followup

>>
>>
>> + 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).
>

Ok done

>>
>>      `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