shopinvader_api_address: support partial update

[sebastienbeau]

Add partial update on address (see here : https://github.com/shopinvader/odoo-shopinvader/pull/1425/files#r1361596661)
@sbidoul @lmignon

[sbidoul]

IIRC the full update was a conscious decision in #1361, but I can't find the discussion right now.

How do you reset values to null with this approach?

What is the benefit of a partial update?

If we do this, this must be explained clearly in the service docs visible in swagger.

[lmignon]

@sbidoul The reset of value is easy. If a key for a field is into the json rqst, that means that a new value is provided and must be set as new value (even if null)...

In the schema definition, we can specify that a field is required and nullable. Required means that the field must be present into the json. Nullable applies to the provided value.....

    # name is optional into the request and can be null
    name: str | None = None

   # name is required into the request and can be null
    name: str | None

   # name is optional into the request but when provided can't be null
    name: str = None

   # name is required into the request but when provided can't be null
    name: str

The call to the method self.model_dump(exclude_unset=True) return a dictionary with only fields provided into the json....

# nullable and optional
class A(BaseModel):
    name: str | None = None

print(A().model_dump(exclude_unset=True))
>> {}
print(A(name=None).model_dump(exclude_unset=True))
>> {'name': None}
print(A(name="a").model_dump(exclude_unset=True))
>> {'name': 'a'}


# nullable and required
class A(BaseModel):
    name: str | None

print(A().model_dump(exclude_unset=True))
>> pydantic_core._pydantic_core.ValidationError: 1 validation error for A
>> name
>>  Field required [type=missing, input_value={}, input_type=dict]
>>    For further information visit https://errors.pydantic.dev/2.3/v/missing
print(A(name=None).model_dump(exclude_unset=True))
>> {'name': None}
print(A(name="a").model_dump(exclude_unset=True))
>> {'name': 'a'}


# not nullable and required
class A(BaseModel):
    name: str

print(A().model_dump(exclude_unset=True))
>> pydantic_core._pydantic_core.ValidationError: 1 validation error for A
>> name
>>  Field required [type=missing, input_value={}, input_type=dict]
>>    For further information visit https://errors.pydantic.dev/2.3/v/missing
print(A(name=None).model_dump(exclude_unset=True))
>> pydantic_core._pydantic_core.ValidationError: 1 validation error for A
>> name
>>  Input should be a valid string [type=string_type, input_value=None, input_type=NoneType]
>>   For further information visit https://errors.pydantic.dev/2.3/v/string_type
print(A(name="a").model_dump(exclude_unset=True))
>> {'name': 'a'}


# not nullable and optional
class A(BaseModel):
    name: str = None

print(A().model_dump(exclude_unset=True))
>> {}
print(A(name=None).model_dump(exclude_unset=True))
>> pydantic_core._pydantic_core.ValidationError: 1 validation error for A
>> name
>>  Input should be a valid string [type=string_type, input_value=None, input_type=NoneType]
>>   For further information visit https://errors.pydantic.dev/2.3/v/string_type
print(A(name="a").model_dump(exclude_unset=True))
>> {'name': 'a'}

That's being said, IMO the definition of the AddressUpdate schema is not right

class AddressUpdate(StrictExtendableBaseModel):
    """
    used to update address (res.partner)
    state and country can be name or code
    """
    name: str | None = None
    street: str | None = None
    street2: str | None = None
    zip: str | None = None
    city: str | None = None
    phone: str | None = None
    email: str | None = None
    state_id: int | None = None
    country_id: int | None = None

At least the name can't be null.

class AddressUpdate(StrictExtendableBaseModel):
    """
    used to update address (res.partner)
    state and country can be name or code
    """
    name: str  = None
    ...

[sbidoul]

Ok, thanks for the detailed explanation.

This still raises some questions in my head:

• empty vs null vs not set may be complicated to grasp
• having to dump back to a JSON dict is weird, and we loose rich types when doing that (e.g. dates)
• do we have use cases? is it not easier for the front to send all values always?
• this is easy to break inadvertently just with subtle changes in the schema
• this would need tests and docs

[sbidoul]

do we have use cases

Forcing the front to pass the email when updating the invoicing address may be a problem.

[lmignon]

Ok, thanks for the detailed explanation.

This still raises some questions in my head:

• empty vs null vs not set may be complicated to grasp

It's true at first and could lead to error for beginners.

• having to dump back to a JSON dict is weird, and we loose rich types when doing that (e.g. dates)

model_dump doesn't dump to JSON. It dumps python value. (model_json_dum dumps to JSON)

class B(BaseModel):
    f: float
    dt: datetime
    d: date

print(B(f="5.1", d="2023-11-03", dt="2023-11-03T11:12:12Z").model_dump(exclude_unset=True))

>> {'f': 5.1, 'dt': datetime.datetime(2023, 11, 3, 11, 12, 12, tzinfo=TzInfo(UTC)), 'd': datetime.date(2023, 11, 3)}

• do we have use cases? is it not easier for the front to send all values always?

??

• this is easy to break inadvertently just with subtle changes in the schema

Why? We still need to define the conversion from the schema to the value to write.

• this would need tests and docs

For sure

[sbidoul]

model_dump doesn't dump to JSON. It dumps python value. (model_json_dum dumps to JSON)

Ah, TIL, thanks!

Ok then. So do we want to set this as a pattern that all shopinvader update services should work this way?

[lmignon]

model_dump doesn't dump to JSON. It dumps python value. (model_json_dum dumps to JSON)

Ah, TIL, thanks!

Ok then. So do we want to set this as a pattern that all shopinvader update services should work this way?

Pros:

• Less constraining for client applications -> add more flexibility in the way to design the updates on service consumer

Less:

• Requires greater skills in the advanced features offered by Pydantic.
• Less explicit and less traceable approach of the use of attributes defined by the schema

The last point can be addressed by adopting a different pattern for processing the data received.

def to_res_partner_vals(self) -> dict:
        values = self.model_dump(exclude_unset=True)
        vals = {}
        if "name" in values:
            vals["name"] = self.name
       if "street" in values:
           vals["street"] = self.street
       ...
       return vals

Personally, I am in favour of adopting as a guideline the development of update services that allow partial updating.

ping @AnizR @qgroulard @simahawk @hparfr @marielejeune @GuillemCForgeFlow @cristiano-one

[AnizR]

Personally, I am in favour of adopting as a guideline the development of update services that allow partial updating.

I think that partial update can be really useful and I am not against it!

Shouldn't we make a distinction between partial update and total update?

This could be done using the POST (partial update) and the PUT (total update) method.

[lmignon]

/ocabot merge patch

[shopinvader-git-bot]

On my way to merge this fine PR!
Prepared branch 16.0-ocabot-merge-pr-1443-by-lmignon-bump-patch, awaiting test results.

[shopinvader-git-bot]

@lmignon your merge command was aborted due to failed check(s), which you can inspect on this commit of 16.0-ocabot-merge-pr-1443-by-lmignon-bump-patch.

After fixing the problem, you can re-issue a merge command. Please refrain from merging manually as it will most probably make the target branch red.

[simahawk]

Personally, I am in favour of adopting as a guideline the development of update services that allow partial updating.

I agree. Had to fight we this in past implementations.

This could be done using the POST (partial update) and the PUT (total update) method.

Good point.

[sbidoul]

I'm investigating the test failure. Probably related to OCA/rest-framework#402

[sbidoul]

Tests fixed in #1486

[sebastienbeau]

/ocabot merge patch

[shopinvader-git-bot]

Hey, thanks for contributing! Proceeding to merge this for you.
Prepared branch 16.0-ocabot-merge-pr-1443-by-sebastienbeau-bump-patch, awaiting test results.

[shopinvader-git-bot]

It looks like something changed on 16.0 in the meantime.
Let me try again (no action is required from you).
Prepared branch 16.0-ocabot-merge-pr-1443-by-sebastienbeau-bump-patch, awaiting test results.

[shopinvader-git-bot]

Congratulations, your PR was merged at d568329. Thanks a lot for contributing to shopinvader. ❤️