JSON application messaging in Plone core?

Development
1

It's very nice that we have improved REST support in Plone core, in the form of plone.rest etc.

If I understand correctly, plone.rest intercepts during traversal all requests with Accept: application/json header, to do its magic (although it has some exceptions built in for some special cases).

Because of this, some features in Plone TTW theme editing have been broken ever since plone.rest was merged into core. A simple short fix has been proposed a few times over the last couple of years, but it's been repeatedly refused & suggested that the issue should be resolved by changes elsewhere in Plone instead.

Trying to help out, I went down the rabbit hole to see if broken TTW theme file upload could be fixed by changing upload so that rather than JSON messaging, it'd rely on normal HTTP/REST request/response semantics. Upload requests would then no longer be caught by plone.rest.

I checked plone.app.theming (the easy part), thememapper -> filemanager -> upload patterns, and third-party Dropzone.js that ultimately does the upload. Saw that it could be done in a way that makes sense. Maybe just 20-30 lines of functional code changes on both sides, in total. But of course there are also tests that'd need to be changed. Both in backend & frontend. Perhaps something else unforeseen too; there usually is.

So, let's presume someone is willing to spend time on this effort, even whilst knowing that the same outcome could be accomplished in 5 minutes by three or so lines in plone.rest.

But... this is just for one use case of one pattern. I did a quick search in mockup and it seems there are at least six patterns that use JSON application messaging of some sort. Which means they likely use Accept: application/json, which means... you get the picture. I wonder how many bugs we have in Plone because of the same root cause?

I might even be willing to go & create PRs for both mockup & plone.app.theming to resolve the TTW upload issue. If it was only that. But I am afraid that I'd probably soon find the next issue in theming caused by the same thing, and then the next, ... A lot of work in total. And I cannot help but think that addressing these cases would probably be trivial if only it was ok to add exceptions for them in somewhere else...

Maybe the patterns that use JSON messaging all should be changed by some objective measure; HTTP/REST compliance & best practices or such, as it has been suggested. Unrelated to the root of the issue as those measures might be. And maybe adding a few more exceptions to plone.rest traversal intercept logic would have some significant drawbacks. I don't know what they'd be.

But it seems there is a big known cost for Plone on the other side: things are broken now, and would perhaps remain so forever, given the possibility that nobody is going to do this non-trivial amount of non-trivial work. We might then as well e.g. cut off TTW theme editing completely for example. It wouldn't be the first time Zope & Plone communities have killed TTW stuff over the years, by people who had no need for it.

Personally, I would appreciate a economical and practical solution that can be worked on. So that we could keep TTW theming and whatever else that might be affected.

I don't really know where else I should've raised this dilemma. I would imagine this might have been discussed already somewhere, maybe several times. And given that attempting to use JSON over HTTP for application messaging forces Plone core and add-ons to use plone.rest or not use JSON messaging at all, might there be some sort of accepted plan or policy somewhere? With clear direction or guidance on this matter? If not, should something like that be formulated?

Thanks for reading. Apologies for any mistakes & incorrect observations. My history with Zope & Plone is, while surprisingly long by now, somewhat superficial and more occasional than regular. So bear with me. Corrections welcome.

6 Likes
2

Thank you for writing this down. To be honest I was under the assumption that the usage of the application/json header was only a single occurrence and not a pattern that Mockup was using. I was also under the assumption that this would be easy to fix in mockup. Apart from the very first development during sprints I never touched or used Mockup or Patternslib at all, so please excuse my ignorance here.

I was reluctant (and I am still somehow are) to add some hacky workaround code to a package to fix code from another package that violates common standards. Though, of course, everything is a tradeoff and I fully understand your point.

Maybe some of the folks who actively develop Mockup can step up here and give their opinions. I know that @thet worked on modernizing Mockup and fixing those things would be a base requirement to being able to use plone.rest(api) with Mockup in the future. If we want to have a classic Plone LTS in Plone 6, this should also be part of this work IMHO.

Adding workarounds in plone.rest is a slippery slope and hacky workarounds in plone.rest can have negative side effects as well. So we have to look into this in more detail and find a solution. I guess it would help to figure out where we use the application/json header in Mockup elsewhere to get an idea of what we are up to.

3

@petri @tisto

@petri can you point me to some code where you proposed the fix in plone.rest and mockup?

I'd naively expect that requests for appilcation/json which fail with plone.rest would fall back to standard plone traversing mechanisms. Isn't that possible? That would be the easiest fix.

In mockup I want as much JSON request as possible to use plone.restapi. The structure and related items patterns are good good example, the upload pattern maybe another. But still, this probably doesn't make sense everywhere.

Also apart from Mockup I have often used application/json requests for custom browser views - as probably do a lot of Plone addons. I'd like to see such requests still working with Plone 5.2.

That calls for a debugging session.

4

@thet when traversing to the theme file upload browser view, portal_resources is on the path and cannot be traversed to. KeyError is raised for it at the end of the plone.rest traversal hook.

It's been suggested to let traversal pass by unharmed, in this case, by e.g. adding an exception for portal_resources, similar to what's there for VirtualHostMonster.

In mockup, I was thinking about adding an utility to filemanager pattern to support overriding headers that Dropzone.js sets:

    setUploadHeader: function(name, value) {
      var self = this;
      var view = self.getUpload();
      if (view !== undefined) {
        view.upload.dropzone.options.headers[name] = value;
      }
    },

and then use it from thememapper pattern, e.g.:

self.fileManager.setUploadHeader('Accept', '*/*');  // no 'application/json'

However, all things considered, rather than avoiding using JSON messaging in fear of pre-emptive traversal interrupt by plone.rest, I guess we should indeed just embrace it & convert JSON-messaging Plone core backends (BrowserViews) into plone.rest Services, and add them to plone.restapi.

But that does not help if those Services cannot be traversed to in the first place... :slight_smile:

1 Like
5

After re-familiarizing & educating myself with zope & plone.rest traversal details some more, here's what appears to me to be going on:

  • The issue of broken theme file uploads etc. is not really related to Accept: header usage by p.a.t or mockup at all... it's just a symptom and focusing on that has been a misguided sidetrack.
  • The root cause is that plone.rest has to adapt, hook, monkeypatch, inject and hack itself into traversal core from the outside, in order to deliver. Given complexity of traversal (there's tons of variations & combinations), it is very hard for plone.rest to cover all the cases that exist - such as traversal to the p.a.t file upload service through some BTree2 folders & a custom theme traverser.
  • Furthermore, working from outside to override & accommodate a complex set of cases makes plone.rest overly complex (by necessity) for what it tries to deliver; it should be re-implemented as a very focused addition to Zope core instead. At the moment it is instead (in effect) reimplementing Zope traversal to a (way too big a) degree. It is brittle and error-prone and will become more so - just like Zope traversal. No wonder adding support for this far overlooked cases in plone.rest is painful and difficult.

This whole thing is illustrated quite effectively by the fact that reimplementing p.a.t theme file upload BrowserView as a plone.rest Service does not solve anything, because plone.rest does not support traversal to it when registered for & accessed at the same path as the browser view.

1 Like
6

I fully agree that traversal and REST is overly complex and very hard to grasp.

Just a bit of background: the implementation that we have today in plone.rest is the third or fourth iteration and full rewrite of a painful and overly complex endeavour to somehow hack a RESTful service into Zope / Plone. It required a significant amount of work from and with some of the smartest people I know in the Plone community.

The ZServer code that handles all this is among the most oldschool code (I will not rant) I personally ever saw and it hasn't been touched by anyone since forever (at least not when we looked into it). I recall for instance that ZServer just assumes that any PATCH call goes to Webdav...

Our original plan was that at some point plone.rest code would go into ZServer (before we started to rip out ZServer). Though, I personally have other priorities right now and things are different after the Python 3 migration anyways.

If you want to tackle this, please go ahead and propose and implement something better.

7

I'd be open to see if we can remove the raise and see if we can pass the traversal. To be honest it has been a while since I looked at that code and as said it was refactored multiple times from the scratch. First by @ramon and then by @buchi. Unfortunately I do not have enough time to look into this issue right now.

Maybe @thet can help here or @buchi with the plone.rest part.

plone.rest was build under the assumption that no code in Plone uses JSON. This was obviously wrong and I fully agree with you folks that we should start using plone.restapi in mockup/patternslib. Though, since I lack experience and time, I won't be much help here.

If we do not find a solution, I would consider merging the original workaround. As said, my assumption was that this is something that is fixable in Plone and in the original issue I did not get the impression that the reporter really tried to fix this in the way that @petri is going now and I really appreciate how much effort he already put into this painful and frustrating issue.

8

I absolutely do not question the complexity of the problem and the implementation is very clever, covers the whole REST problem fields including CORS negotiation and works in production.

However, I try to look into the traversal problem tomorrow and if that leads to no ends, maybe we can really add some exceptions to it, maybe configurable.

1 Like
9

Update on this issue: I have a similar issue with fileUpload on lineage subsites when served via an virtual host root (nginx or an entry in VHM).
POST requests to fileUpload fail with 404. Documented here: https://github.com/plone/plone.restapi/issues/680

The best fix at the moment would be to change the POST URL to @@fileUpload. This works because of an already existing exception in https://github.com/plone/plone.rest/blob/878ff92054a9f84a2f66ded1a16c35129ed17dd4/src/plone/rest/traverse.py#L41

In the long run we should register plone.rest services for all application/json requests. This is what it is meant for. As a consequence we probably need to install plone.rest by default.

10

I took a stab at simplifying the plone.rest traversal hijack implementation, cutting traverse.py down to 65 lines (from 110). Looks like (traversal to) the theme file upload BrowserView works with it now.

All tests pass on Plone 5.2 & Python3.7 so I am hopeful this effort might serve at least as an inspiration for someone more knowledgeable who could carry this on so we'd have some real progress. But I might well have missed something that renders the whole simplification strategy useless. And of course there might be gaps in the test suites...?

Would be great if someone took a look. The idea behind the simplification was to eagerly release traversal control back after doing the minimum necessary. For some use cases this may well now be too simple, too little...

2 Likes
11

I opened a PR for your changes to better track and discuss them: https://github.com/plone/plone.rest/pull/105

Quite an impressive PR! It leaves not much in traverse. I'm looking forward to see if it really will fix the issues and doesn't introduce new ones.

1 Like
12

Hi ... since I've got the same problem with restapi and lineage subsite I took a second look at the plone.rest traversal refactoring. The simplifications of @petri where quite there, but the wrapper object had some missing bits ... I also wrote a test for this scenario ... github actions are green ... jenkins is running.