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

PUT on productclasses #341

Open
JulienPalard opened this issue Apr 2, 2024 · 6 comments
Open

PUT on productclasses #341

JulienPalard opened this issue Apr 2, 2024 · 6 comments

Comments

@JulienPalard
Copy link
Contributor

JulienPalard commented Apr 2, 2024

I'm trying to update productclasses attributes via the admin API, my first test was to GET and PUT a document just to check if it rounds-trip, but it won't. I then tried to patch the 'attributes' attribute without changing it, like:

        product_class = session.get("url of a product class").json()
        response = session.patch(
            product_class["url"], json={"attributes": product_class["attributes"]}
        )
        print(response.json())

but it gives:

{'attributes': [{'product_class': ['This field is required.']}, {'product_class': ['This field is required.']}, {'product_class': ['This field is required.']}]}

which feels broken?

@specialunderwear
Copy link
Member

specialunderwear commented Apr 2, 2024

Yes that is definately a bug. But I'm also interested in what is not working with PUT when you PUT the same content returned from GET. Are you only using packages released on pypi?

@JulienPalard
Copy link
Contributor Author

Tried with more time today a clean repro of the PUT issue and was not able to reproduce it:

import argparse
from urllib.parse import urljoin
import json
import requests


def parse_args():
    parser = argparse.ArgumentParser()
    parser.add_argument("username")
    parser.add_argument("password")
    parser.add_argument("--api", default="http://localhost:8000/api/")
    args = parser.parse_args()
    if not args.api.endswith("/api/"):
        raise ValueError("API URL is expected to end with /api/")
    return args


def main():
    args = parse_args()
    session = requests.session()
    session.auth = (args.username, args.password)
    productclasses = session.get(urljoin(args.api, "admin/productclasses/"))
    productclass = productclasses.json()["results"][0]
    print(json.dumps(productclass, indent=4))
    productclass["name"] += "a"
    response = session.put(productclass["url"], productclass)
    print(response)

It just works.

I then tried a clean repro of the PATCH one, and it fails:

import argparse
from urllib.parse import urljoin
import json
import requests


def parse_args():
    parser = argparse.ArgumentParser()
    parser.add_argument("username")
    parser.add_argument("password")
    parser.add_argument("--api", default="http://localhost:8000/api/")
    args = parser.parse_args()
    if not args.api.endswith("/api/"):
        raise ValueError("API URL is expected to end with /api/")
    return args


def main():
    args = parse_args()
    session = requests.session()
    session.auth = (args.username, args.password)
    productclasses = session.get(urljoin(args.api, "admin/productclasses/"))
    productclass = productclasses.json()["results"][0]
    response = session.patch(
        productclass["url"], json={"attributes": productclass["attributes"]}
    )
    print(response.json())


main()

gives as stated in the first message:

{'attributes': [{'product_class': ['This field is required.']}, {'product_class': ['This field is required.']}, {'product_class': ['This field is required.']}, {'product_class': ['This field is required.']}, {'product_class': ['This field is required.']}, {'product_class': ['This field is required.']}, {'product_class': ['This field is required.']}, {'product_class': ['This field is required.']}, {'product_class': ['This field is required.']}, {'product_class': ['This field is required.']}, {'product_class': ['This field is required.']}, {'product_class': ['This field is required.']}, {'product_class': ['This field is required.']}, {'product_class': ['This field is required.']}, {'product_class': ['This field is required.']}, {'product_class': ['This field is required.']}, {'product_class': ['This field is required.']}, {'product_class': ['This field is required.']}, {'product_class': ['This field is required.']}, {'product_class': ['This field is required.']}, {'product_class': ['This field is required.']}, {'product_class': ['This field is required.']}]}

I'll have to test with a clean install from pip to ensure I did not changed too many things. At least I don't have anything overriden for oscarapi.serializers.admin.product except an AdminProductserializer.

@JulienPalard
Copy link
Contributor Author

Looks like it happen before the update call in the AdminProductClassSerializer, so during validation.

@JulienPalard
Copy link
Contributor Author

JulienPalard commented Apr 3, 2024

Oh, looks like it comes from a UniqueTogetherValidator between code and product_class:

The `UniqueTogetherValidator` always forces an implied 'required'
state on the fields it applies to.

Which itself is autogenerated from the AbstractProductAttribute,Meta.unique_together (from oscar) by the ModelSerializer.

I'm no rest_framework guru, but a fix could be to stuff product_class into all attributes before validation. If done in the AdminProductClassSerializer it can even make sense: as the attribute is inside a productclass it is obviously linked to this productclass. But should it be done in the view instead?

Hum there may be multiple view with the issue (the list one accepting create and the detail one accepting updates). Maybe it's less redundent to put this in the serializer. But I don't see an elegant place to put this, maybe by overriding run_validation? But we don't have the product_class instance soon enough... damned validation is complicated.

Another "good" place may be inside ProductAttributeSerializer, teaching it to understand being nested in an AdminProductClassSerializer, but again, in its run_validators method, the self.parent.parent (AdminProductClassSerializer) has not yet been validated...

@specialunderwear
Copy link
Member

try sending more data. this is still a bug but I think you can test which data is required to be sent to make patch not fail.
Try sending the same data as returned from the GET, and remove data untill it breaks.

@JulienPalard
Copy link
Contributor Author

Sending more data does not helps, up to the whole document, see this reproducer:

import argparse
from urllib.parse import urljoin
import json
import requests


def parse_args():
    parser = argparse.ArgumentParser()
    parser.add_argument("username")
    parser.add_argument("password")
    parser.add_argument("--api", default="http://localhost:8000/api/")
    args = parser.parse_args()
    if not args.api.endswith("/api/"):
        raise ValueError("API URL is expected to end with /api/")
    return args


def main():
    args = parse_args()
    session = requests.session()
    session.auth = (args.username, args.password)
    productclasses = session.get(urljoin(args.api, "admin/productclasses/"))
    productclass = productclasses.json()[0]
    print(json.dumps(productclass, indent=4))
    response = session.put(
        productclass["url"], json=productclass
    )
    print(response.json())


main()

either with a put or with a patch I'm getting:

{'attributes': [{'product_class': ['This field is required.']}]}

it has been tried from a freshly created sandbox (just added a user to access /api/admin/).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants