feat: 1055 - new experimental class "FlexibleProduct"

[monsieurtanuki]

What

This PR adds flexibility:

• we can now use "double" API version number, e.g. 3.2
• we can now extract product data from json on the fly, instead of formatting the json into a Product before anything else

With that PR we can easily accept new product fields and fields changing format, as we can just store the json regardless of the current or future product format.

Fixes bug(s)

• Closes: Add flexibility to Product according to api version #1055

Part of

• Add a numeric field in dev mode to be able to use an arbitrary version of the API smooth-app#6502

Files

New files:

• api_get_flexible_product.dart: tests around the new class FlexibleProduct.
• flexible_map.dart: (experimental) helper class around json and maps.
• flexible_product.dart: (experimental) Product without predefined structure, relying mainly on the json.
• flexible_product_result.dart: (experimental) API answer from a call to /api/v???/product/$barcode, in a flexible manner.
• flexible_search_result.dart: (experimental) API answer from a call product search, in a flexible manner.

Impacted files:

• language_helper.dart: added 3 experimental methods for FlexibleProduct
• open_food_api_client.dart: added 2 experimental methods for FlexibleProduct - getFlexibleProductResult and searchFlexibleProducts
• openfoodfacts.dart: added the new 4 class files
• product_image.dart: added an experimental method to compute the image URLs
• product_query_configurations.dart: made it possible to call API get product method with a "double" API version number - and not only an "int"
• product_result_v3.dart: minor refactoring
• product_result_v3.g.dart: generated

[monsieurtanuki]

@teolemon ping

[monsieurtanuki]

@teolemon ping

ping

[monsieurtanuki]

@teolemon ping

ping

ping

[teolemon]

Sorry @monsieurtanuki , underwater on non mobile things. I asked for a technical quick look from @PrimaelQuemerais @g123k (who also seem to be a little bit underwater).

[monsieurtanuki]

@teolemon For the record it is related to the app, cf. openfoodfacts/smooth-app#6502
I don't know if that's a priority, though.

[monsieurtanuki]

@teolemon @g123k @PrimaelQuemerais ping

[monsieurtanuki]

Closed as stale.

[monsieurtanuki]

Ping

[PrimaelQuemerais]

Hello @monsieurtanuki
The code looks good. I'm wondering what do you plan on using this for?
You give this example : The typical use case would be "extending" this class in order to control brand new field values.
But if we still update a class to add new fields why not add these new fields to Product directly?

[monsieurtanuki]

@PrimaelQuemerais The typical use case is that 'brands' are a comma-separated String in OxF, and a List in Searchalicious.
With the FlexibleProduct we don't give a damn: we just store a JSON locally and extract the brands one way or the other. In the current system, we need the Product to match exactly what we expect, format-wise. With the JSONSerializable syntax on top, which means we cannot support both data types at the same time.

Another use case is fields that we download but ignore forever because at the time of the download they weren't part of the Product structure.

And with an 'extends' it's easy to override or add fields.

[monsieurtanuki]

But if we still update a class to add new fields why not add these new fields to Product directly?

To answer that point more specifically, we've been asked to be more reactive, e.g. for new experimental fields.
If we keep the current system, we have to create a new version of off-dart first, with experimental code. With FlexibleProduct, we just have to extend FlexibleProduct - e.g. in Smoothie.

[PrimaelQuemerais]

I understand, thank you for the prompt reply.
Let me read through the code once more to check if I see anything

[PrimaelQuemerais]

Following on your example with brands.
Since FlexibleProduct is supposed to be used with both the OxF API and Searchalicious, wouldn't this throw if json['brands'] is a List instead of String?

[monsieurtanuki]

@PrimaelQuemerais That's correct. But it's not relevant for the moment to implement anything specific to Searchalicious.
That first PR - that already started more than 3 months ago - is only about creating a FlexibleProduct that works like a Product.

[PrimaelQuemerais]

But if it doesn't implement this kind of logic I don't understand where this model stands between Product and storing the product in a Map. Do you already know how this would be handled in FlexibleProduct and why it couldn't be implemented in Product directly?

JSONSerializable still allows us to implement multiple parsing logics as long as the returning type remains the same, and wouldn't you face the same issue with FlexibleProduct?

Let me know if there's something I'm not getting here.

[monsieurtanuki]

as long as the returning type remains the same

That's the problem for "brands".

No format problem will be faced with FlexibleProduct, assuming it's correct, JSON-wise.
What we cannot do today with Product is accepting a priori data that don't match exactly requirements decided months ago.
Again, currently when we download products, we first check if they match the expected Product structure. If they don't, we ignore them by throwing an exception.
With FlexibleProduct, we won't have this problem because as long as it's JSON, we accept the data. We'll still have to decide later how to retrieve this or that field - e.g. for brands - but the data won't be rejected a priori. And extending FlexibleProduct e.g. in Smoothie will make it possible to be flexible about the data.

Basically that would be something like

@override
String? get brands {
  dynamic result = json['brands'];
  if (result == null) {
    return null;
  }
  try {
    return result as String;
  } catch(e) {
    return (result as List<String>).join(',');
  }
}

[PrimaelQuemerais]

We can implement the same try catch in the Product deserialization logic which I think is what we should do.

This FlexibleProduct approach seems to introduce a lot of runtime errors as no-one expects a getter to throw.
If we want to manipulate products from the server without flexibly I think it is wiser to keep it as a Map until we are sure of the structure, at which point it should be deserialization into an object, but then I don't see what prevents us from implementing everything into Product directly.

That's my take but it would be nice to get more opinions @g123k @teolemon

[monsieurtanuki]

Okay let's reset and let me try to understand you better.

Cool

Is the goal of FlexibleProduct to provide developers a way to get products from the API regardless of the structure of what the API sends back?

Correct. In order to provide flexibility to API structure changes.

And it is better than a function returning a Map<String, dynamic> because it offers default getters?

Correct. So that developers don't have to reinvent the wheel for the 99% for fields that don't change.

[PrimaelQuemerais]

I'll take a bit more time to ponder over this but here's a few thoughts :

One improvement I would make then is to allow developers to pass their own type to getFlexibleProductResult() like this : static Future<FlexibleProductResult> getFlexibleProductResult<T extends FlexibleProduct>(...). This way FlexibleProductResult can contain a list of their own type. And same for searchFlexibleProducts().

One concern is if I extend FlexibleProduct to let's say fix the issue with brands being Strings or Lists, I won't be able to use the brand attribute as dart doesn't allow me to both override and change the type at the same time. How to we expect developers to work around this?

I still don't really like having getters which can throw, but that's a design choice and no solution will be perfect for this problem anyway, but maybe we can find a way to let developers know that these getters can be unsafe? I can take some time to think about it.

[monsieurtanuki]

One improvement I would make then is to allow developers to pass their own type to getFlexibleProductResult() like this : static Future<FlexibleProductResult> getFlexibleProductResult<T extends FlexibleProduct>(...). This way FlexibleProductResult can contain a list of their own type. And same for searchFlexibleProducts().

I think that would be over-engineering the code:

• we already have access to json, so we can build whatever class on top of that json if needed
• in dart we can extend FlexibleProduct whenever we want, including in Smoothie, so I can't think of anything we couldn't fix with an extension

One concern is if I extend FlexibleProduct to let's say fix the issue with brands being Strings or Lists, I won't be able to use the brand attribute as dart doesn't allow me to both override and change the type at the same time. How to we expect developers to work around this?

I still don't really like having getters which can throw, but that's a design choice and no solution will be perfect for this problem anyway, but maybe we can find a way to let developers know that these getters can be unsafe? I can take some time to think about it.

In order to solve this issue forever, the idea would be to deprecate brands and to create 2 new getters, approximately coded like that:

  String? get brandsAsString {
    final result = json['brands'];
    if (result == null) {
      return null;
    }
    if (result is String) {
      return result;
    }
    return (result as List<String>).join(',');
  }

  List<String>? get brandsAsList {
    final result = json['brands'];
    if (result == null) {
      return null;
    }
    if (result is List<String>) {
      return result;
    }
    return (result as String).split(',');
  }

What do you think?

[PrimaelQuemerais]

Correct me if I'm wrong but in order to deprecate brands, one would need to create a new class extending from FlexibleProduct. Thus an extension wouldn't be enough?

[monsieurtanuki]

@PrimaelQuemerais Indeed, we don't need brands after all. With brandsAsString and brandsAsList we're good.

[monsieurtanuki]

@PrimaelQuemerais With my latest push:

• added the concept of flexibleFields - when the server provides new fields, we can query them before off-dart's ProductFields include those new fields
• added SCHEMA_VERSION as a product field
• changed getter brands into less ambiguous getters brandsAsString and brandsAsList

[monsieurtanuki]

@PrimaelQuemerais ping

[monsieurtanuki]

Closed as stale. Again.