Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix jsonapi.read omits include_methods when a single parameter is used #1493

Merged
merged 4 commits into from
Dec 23, 2019

Conversation

xispa
Copy link
Member

@xispa xispa commented Dec 20, 2019

Description of the issue/feature this PR addresses

When include_methods param is used in a read call with a single parameter (instead of a list of more than one), the jsonapi does not handle the value parameter properly and transforms it to a list of characters instead.

A call like follows:

this.ajax_submit({
  url: this.get_portal_url() + "/@@API/read",
  data: {
          catalog_name: "uid_catalog",
          UID: widget.attr('data-uid'),
          include_methods: ["getSomething"],
        }
}).done(function(data) {
  return deferred.resolveWith(this, [data.objects[0]["getSomething"]]);
});

fails. The function get_include_methods returns getSomething instead of a list [getSomething]. Therefore, the function load_method_values tries the following calls to the instance: g, e, t, S, o, ...

If the call is like follows:

this.ajax_submit({
  url: this.get_portal_url() + "/@@API/read",
  data: {
          catalog_name: "uid_catalog",
          UID: widget.attr('data-uid'),
          include_methods: ["getSomething", "getId"],
        }
}).done(function(data) {
  return deferred.resolveWith(this, [data.objects[0]["getSomething"]]);
});

the list is resolved properly.

Current behavior before PR

read call fails when using include_methods param with a single value

Desired behavior after PR is merged

read calls does work when using include_methods param with a single value

--
I confirm I have tested this PR thoroughly and coded it according to PEP8
and Plone's Python styleguide standards.

When include_methods param is used in a read call with a single
parameter (instead of a list of more than one), the jsonapi does
not handle the value parameter properly and transforms it to
a list of characters instead.

A call like follows:

```
this.ajax_submit({
  url: this.get_portal_url() + "/@@API/read",
  data: {
          catalog_name: "uid_catalog",
          UID: widget.attr('data-uid'),
          include_methods: ["getSomething"],
        }
}).done(function(data) {
  return deferred.resolveWith(this, [data.objects[0]["getSomething"]]);
});
```

fails. The function `get_include_methods` returns `getSomething`
instead of a list `[getSomething]`. Therefore, the function
`load_method_values` tries the following calls to the instance:
`g`, `e`, `t`, `S`, `o`, ...

If the call is like follows:
```
this.ajax_submit({
  url: this.get_portal_url() + "/@@API/read",
  data: {
          catalog_name: "uid_catalog",
          UID: widget.attr('data-uid'),
          include_methods: ["getSomething", "getId"],
        }
}).done(function(data) {
  return deferred.resolveWith(this, [data.objects[0]["getSomething"]]);
});
```
the list is resolved properly.
@xispa xispa requested a review from ramonski December 20, 2019 22:33
@xispa xispa added the Bug 🐞 label Dec 20, 2019
return include_methods
include_methods = request.get("include_methods[]")
if not include_methods:
include_methods = request.get("include_methods", [])
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When is the variable named include_methods[] in the request?

Copy link
Member Author

@xispa xispa Dec 22, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

First of all, include_methods is not used as a parameter anywhere in the code, but I was playing around and found this issue. Nevertheless, include_fields from here:

@ajax_submit
url: @get_portal_url() + "/@@API/read"
data:
catalog_name: "uid_catalog"
UID: widget.attr('data-uid')
include_fields: [fieldname]
.done (data) ->
return deferred.resolveWith this, [data.objects[0][fieldname]]
return deferred.promise()

is received as include_fields[]. Therefore, this trick here:

if "include_fields[]" in request:
include_fields = request['include_fields[]']
return include_fields

Same happens if you use include_methods instead. Thus, I just followed same criteria and did not go any further. As a side note, the issue this PR addresses happens with include_fields too, but is a hidden "bug" because of:

if include_fields and fieldname not in include_fields:
continue

and this code will work even if include_fields is a string instead of a list.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, I think I got it. This means that either JQuery or the Zope Publisher is converting include_methods -> include_methods[]?

Anyhow, going to merge then.

@ramonski ramonski merged commit e4f3970 into master Dec 23, 2019
@ramonski ramonski deleted the jsonapi-read-methods branch December 23, 2019 06:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Development

Successfully merging this pull request may close these issues.

2 participants