Making Read-Only Fields Truly Read-Only

[davidpryor]

Hello!

I wanted to gauge the interest in removing the setters for read-only fields. This will protect users from believing a field is used to set some value on the backend.

My thoughts on achieving this is simply moving the current json deserialization to point to a private attribute and exposing this attribute through a getter (basically just removing the setter for the read-only fields).

EXAMPLE: Product class
Before:

  /// Creation timestamp. Read-only.
  @JsonKey(
      name: 'created_t',
      includeIfNull: false,
      fromJson: JsonHelper.timestampToDate,
      toJson: JsonHelper.dateToTimestamp)
  DateTime? created;

After:

  /// Creation timestamp. Read-only.
  DateTime? get created => _created;
  @JsonKey(
      name: 'created_t',
      includeIfNull: false,
      fromJson: JsonHelper.timestampToDate,
      toJson: JsonHelper.dateToTimestamp)
  DateTime? _created;

I know this particular field is one a user may not think about settings, but if we look at
brandsTags - which I believe may be read-only but may be mistaken - a user could believe this is a method of sending their brands in a List instead of a csv String in brands

Just wanted to get some thoughts on this 😁

[M123-dev]

Read only fields are a think we had to deal with a few times in the past

C.f. #291

#682

The first thing here is to find out which fields are the read only ones and then we can think about who to mark this

[davidpryor]

The first thing here is to find out which fields are the read only ones and then we can think about who to mark this

Definitely! And I think this discovery effort and code restructuring could be done in parallel.

Though, removing setters will technically break backwards compatibility, but those setters were never meant to be used 😅

Luckily, this should only potentially break user code since the deserialization is generated automatically.

[monsieurtanuki]

@davidpryor @M123-dev My 2 cents:

• what you haven't considered is that sometimes we need to set fields even if they're not saved - in the Smoothie app when we save data it cannot be immediate, and until we retrieve the saved product from the server we use locally a fake product with fields artificially set.
• we currently use a toJson/fromJson technique to "copy" products
• we need the code to be more resilient (cf. More resilient data decoding (e.g. empty or non-existing fields) #577)

My suggestion:

• throw exceptions/return warnings only when using the saveProduct/V3 method ("hey, those field cannot be saved")

And the "usual suspects":

• "breaking changes" in general is not a desirable goal in itself - it should be worth it
• there are issues with higher priorities

[davidpryor]

Thank you for the response @monsieurtanuki !

"breaking changes" in general is not a desirable goal in itself - it should be worth it

Totally agree! This could go into a major version.

there are issues with higher priorities
I would be willing to write some code around this while you all focus on the higher priority issues.

I think cleaning up some of these interfaces could cause less confusion for new users (like me :) )

what you haven't considered is that sometimes we need to set fields even if they're not saved - in the Smoothie app when we save data it cannot be immediate, and until we retrieve the saved product from the server we use locally a fake product with fields artificially set.

I can definitely see why you might want to do that and I think you could still have this behavior. It just would not be through the main constructor or through directly setting the attribute with what I am proposing.

we currently use a toJson/fromJson technique to "copy" products
This is my thought behind why this proposed direction would not cause issues with serialization since the @JsonKey would be moved to the private member and most of the JsonKeys have explicitly defined names.

[monsieurtanuki]

Hi @davidpryor! Still, I cannot say that I'm a big fan of your suggestion:

• we don't need read-only fields, we need (do we?) to make explicit that only some fields are "savable" - or that some fields are not.
• that could be part of the already existing ProductResultV3, in the warnings section, couldn't it?
• you'd be surprised to see how faster you can code when focusing on higher priority issues first - most of the time you get rid of less important issues because they're not relevant anymore (assuming they were slightly relevant at some point)
• would the code be easier to maintain?

For the record I'm not a big fan of the current implementation, but at least it makes us do whatever we want in Smoothie.

This is what I can suggest:

• have a look at Smoothie and have a look at how Product is used
• code whatever you thought about on Product, and make Smoothie work with your local changes (pubspec.yaml)

  # openfoodfacts: 3.3.2
  openfoodfacts:
    path: ../../../openfoodfacts-dart

• report how much impact your change would have on Smoothie
• please don't put too much energy into it, unless at some point you manage to envision something elegant (easy to understand and to maintain)

[M123-dev]

I guess the main problem with the restriction of some fields is that most of the time the Sdk users do not create a product, they get one from the server and send back an edited one.
So a restriction on the constructor won't have much impact, rather a copyWith and a copyWithRaw

Yes that's more to maintain, but would hide away confusion when an updated product is not returned with the changes.

But again we would first need an overview over all the fields which are writable