feat: Nutrient modifiers (Get/Send product APIs) #1042

g123k · 2025-02-14T09:22:49Z

Hi everyone!

Finally, a working solution for nutrient modifiers, when we get / save a product.

On the GET API, we have _modifier fields.
On the POST one, we must include the modifier character within the value.

monsieurtanuki

Hi @g123k!

I have a doubt about the relevancy of using the same modifier for all PerSizes of a given nutrient:

I'm not sure it's always true
That makes the code harder to read (the keys of the private maps being different), with no added value

Please also read my other comments.

monsieurtanuki · 2025-02-14T14:15:36Z

lib/src/model/nutriments.dart

+    _valueMap[_getTag(nutrient, perSize)] = value;
+    _modifierMap[nutrient.offTag] = modifier;


Could a modifier be different for different PerSizes?

I'm not sure, but I can support it

lib/src/model/nutriments.dart

monsieurtanuki · 2025-02-14T14:25:32Z

test/api_get_product_test.dart

+    expect(nutriments.getModifier(Nutrient.energyKJ), isNull);
+    expect(nutriments.getModifier(Nutrient.energyKCal), isNull);
+    expect(nutriments.getModifier(Nutrient.sugars), isNull);
+    expect(nutriments.getModifier(Nutrient.salt), isNull);
+    expect(nutriments.getModifier(Nutrient.fiber), isNull);
+    expect(nutriments.getModifier(Nutrient.fat), isNull);
+    expect(nutriments.getModifier(Nutrient.saturatedFat), isNull);
+    expect(nutriments.getModifier(Nutrient.proteins), isNull);


Would be more fun if at least one modifier was not null. Or get rid of that test.

My goal was to test a product with no modifier.
But I can remove it, no pb

monsieurtanuki · 2025-02-14T14:26:19Z

test/api_get_product_test.dart

@@ -1430,4 +1432,81 @@ void main() {
      expect(productResult.product!.dataQualityWarningsTags, isNull);
    });
  });
+
+  test('check nutrients modifier (set)', () async {


Can't understand the title, more specifically: "(set)".

I forgot to include a few changes.
Indeed, it wasn't understandable

monsieurtanuki · 2025-02-14T14:28:20Z

test/api_get_product_test.dart

+    expect(nutriments.getModifier(Nutrient.proteins), isNull);
+  });
+
+  test('check nutrients modifier (without)', () async {


Can't understand the title, more specifically: "(without)".

monsieurtanuki · 2025-02-14T14:31:03Z

test/api_save_product_test.dart

@@ -513,5 +518,97 @@ Like that:
      expect(product.noNutritionData, isFalse);
      expect(product.nutriments, isNotNull);
    });
+
+    test('Nutriments modifiers', () async {


The test makes sense, but as you don't save anything (to the server) I guess it should be somewhere else. Or commented as "not really saving anything".
And another test actually saving to the server would make sense.

Co-authored-by: monsieurtanuki <[email protected]>

g123k · 2025-02-15T17:58:24Z

Hi @monsieurtanuki!

Thanks for your review.
We can send a different modifier per 100g or per serving.
However, we only receive one for the two.
I've changed the code to allow this.

monsieurtanuki

Thank you @g123k for your work!
My bad: unfortunately I was a bit wrong in my review, as I didn't get that the server provided only a single modifier for a nutrient, regardless of the PerSize.
Therefore it would make much more sense to remove the PerSize parameter from the _getModifierTag method. Good thing that this method does exist, though, as we just have to remove the parameter and let the syntactic analyzer detect the related errors.

Please have a look at my comments regarding tests: in one case you play the same tests on the same product with the same modifiers. As a consequence you don't really test the "save modifier' feature, you test that it did work once. I suggest that you always change the modifier values, and that you include the null modifier in the "save modifier" test, as it's really a use case.

monsieurtanuki · 2025-02-17T08:43:49Z

lib/src/model/nutriments.dart

+  /// Returns the modifier key for a [nutrient]
+  String _getModifierTag(final Nutrient nutrient, final PerSize perSize) =>
+      '${nutrient.offTag}_${perSize.offTag}';
+


Oh I'm sorry, I've just had a look again at the data provided by the server, I didn't get that the modifier was meant for a nutrient, not for nutrient + persize.
Anyway, that's better to use a method, the impact would be minor on the rest of the code.

Suggested change

/// Returns the modifier key for a [nutrient]

String _getModifierTag(final Nutrient nutrient, final PerSize perSize) =>

'${nutrient.offTag}_${perSize.offTag}';

/// Returns the modifier local key map for a [nutrient]

String _getModifierTag(final Nutrient nutrient) => nutrient.offTag;

monsieurtanuki · 2025-02-17T08:44:16Z

lib/src/model/nutriments.dart

+        _modifierMap[_getModifierTag(nutrient, PerSize.serving)] = modifier;
+        _modifierMap[_getModifierTag(nutrient, PerSize.oneHundredGrams)] =
+            modifier;


Suggested change

_modifierMap[_getModifierTag(nutrient, PerSize.serving)] = modifier;

_modifierMap[_getModifierTag(nutrient, PerSize.oneHundredGrams)] =

modifier;

_modifierMap[_getModifierTag(nutrient] = modifier;

The problem is that the server may have a different value per serving AND 100g.
In this case, the last one will win.

Solution 1: keep the current behavior
Solution 2: save the value depending on the PerSize value of the array.

monsieurtanuki · 2025-02-17T08:46:00Z

lib/src/model/nutriments.dart

+  final Map<String, NutrientModifier> _modifierMap =
+      <String, NutrientModifier>{};


Oops, I did it again: if I read the comment above it makes sense to store null values.

Suggested change

final Map<String, NutrientModifier> _modifierMap =

<String, NutrientModifier>{};

final Map<String, NutrientModifier?> _modifierMap =

<String, NutrientModifier?>{};

monsieurtanuki · 2025-02-17T08:46:40Z

lib/src/model/nutriments.dart

+  NutrientModifier? getModifier(
+          final Nutrient nutrient, final PerSize perSize) =>
+      _modifierMap[_getModifierTag(nutrient, perSize)];


Suggested change

NutrientModifier? getModifier(

final Nutrient nutrient, final PerSize perSize) =>

_modifierMap[_getModifierTag(nutrient, perSize)];

NutrientModifier? getModifier(final Nutrient nutrient) =>

_modifierMap[_getModifierTag(nutrient)];

monsieurtanuki · 2025-02-17T08:47:42Z

lib/src/model/nutriments.dart

+      _valueMap.remove(_getTag(nutrient, perSize));
+      _modifierMap[_getModifierTag(nutrient, perSize)] = modifier!;


Suggested change

_valueMap.remove(_getTag(nutrient, perSize));

_modifierMap[_getModifierTag(nutrient, perSize)] = modifier!;

_valueMap[_getTag(nutrient, perSize)] = null;

_modifierMap[_getModifierTag(nutrient)] = modifier;

monsieurtanuki · 2025-02-17T08:55:55Z

test/api_get_product_test.dart

+    expect(
+      nutriments.getModifier(Nutrient.salt, PerSize.oneHundredGrams),
+      NutrientModifier.lessThan,
+    );
+    expect(
+      nutriments.getModifier(Nutrient.sodium, PerSize.oneHundredGrams),
+      NutrientModifier.lessThan,
+    );


Suggested change

expect(

nutriments.getModifier(Nutrient.salt, PerSize.oneHundredGrams),

NutrientModifier.lessThan,

);

expect(

nutriments.getModifier(Nutrient.sodium, PerSize.oneHundredGrams),

NutrientModifier.lessThan,

);

expect(nutriments.getModifier(Nutrient.salt), NutrientModifier.lessThan);

expect(nutriments.getModifier(Nutrient.sodium), NutrientModifier.lessThan);

monsieurtanuki · 2025-02-17T08:56:52Z

test/api_get_product_test.dart

+    expect(
+      nutriments.getModifier(Nutrient.fiber, PerSize.oneHundredGrams),
+      NutrientModifier.notProvided,
+    );
+    expect(
+      nutriments.getValue(Nutrient.fiber, PerSize.oneHundredGrams),
+      isNull,
+    );
+    expect(
+      nutriments.getValue(Nutrient.fiber, PerSize.serving),
+      isNull,
+    );


Suggested change

expect(

nutriments.getModifier(Nutrient.fiber, PerSize.oneHundredGrams),

NutrientModifier.notProvided,

);

expect(

nutriments.getValue(Nutrient.fiber, PerSize.oneHundredGrams),

isNull,

);

expect(

nutriments.getValue(Nutrient.fiber, PerSize.serving),

isNull,

);

expect(nutriments.getModifier(Nutrient.fiber), NutrientModifier.notProvided);

expect(nutriments.getValue(Nutrient.fiber), isNull);

expect(nutriments.getValue(Nutrient.fiber), isNull);

monsieurtanuki · 2025-02-17T08:57:15Z

test/api_save_product_test.dart

@@ -406,6 +406,11 @@ Like that:
              nutriments.getValue(nutrient, perSize),
              reason: 'should be the same values for $nutrient',
            );
+            expect(
+              nutriments.getModifier(nutrient, perSize),


Suggested change

nutriments.getModifier(nutrient, perSize),

nutriments.getModifier(nutrient),

monsieurtanuki · 2025-02-17T09:00:32Z

test/api_save_product_test.dart

+
+    test('Nutriments modifiers (server call)', () async {
+      Product product = Product(
+        barcode: '1234567890123',


The problem is that you're always setting the same values for the same product. So if you're lucky it may work the first time, and if you're not lucky for some reason it doesn't work anymore (e.g. server somehow changed) and you won't be able to see that.
You may rather start with a getProductV3, and change each modifier.

monsieurtanuki · 2025-02-17T09:01:51Z

test/api_save_product_test.dart

+        result.product!.nutriments?.getModifier(
+          Nutrient.proteins,
+          PerSize.oneHundredGrams,
+        ),
+        NutrientModifier.notProvided,


Rather keeps the nutrient x modifier in a map, then save the product, then get the product, then check that the product matches the map.

monsieurtanuki · 2025-03-05T15:25:03Z

Hi @g123k!

I've just run some tests in PROD on the website, and I don't think there's such thing as "modifier for serving" - it looks like there's only one modifier for both serving and 100g, something like '${nutrient.offTag}_modifier'.
A bit like "unit".

The test was on "salt" for https://world.openfoodfacts.org/api/v3/product/8712000050368/?fields=nutriments

salt "< 0.01" for 100g

      "salt": 0.01,
      "salt_100g": 0.01,
      "salt_modifier": "\u003C",
      "salt_serving": 0.025,
      "salt_unit": "g",
      "salt_value": 0.01,

salt "< 0.026" for serving

      "salt": 0.026,
      "salt_100g": 0.0104,
      "salt_modifier": "\u003C",
      "salt_serving": 0.026,
      "salt_unit": "g",
      "salt_value": 0.026,

salt "> 0.027" for serving

      "salt": 0.027,
      "salt_100g": 0.0108,
      "salt_modifier": "\u003E",
      "salt_serving": 0.027,
      "salt_unit": "g",
      "salt_value": 0.027,

salt "< 0.01" for 100g

      "salt": 0.01,
      "salt_100g": 0.01,
      "salt_modifier": "\u003C",
      "salt_serving": 0.025,
      "salt_unit": "g",
      "salt_value": 0.01,

Support for nutrient modifier (Save/Send product)

fe036bf

g123k requested a review from a team as a code owner February 14, 2025 09:22

github-actions bot assigned g123k Feb 14, 2025

monsieurtanuki reviewed Feb 14, 2025

View reviewed changes

g123k and others added 4 commits February 15, 2025 17:59

Update lib/src/model/nutriments.dart

b806ae1

Co-authored-by: monsieurtanuki <[email protected]>

Update lib/src/model/nutriments.dart

50d0c38

Co-authored-by: monsieurtanuki <[email protected]>

Fix tests

bc96881

Fix tests (bis)

78e5cf0

monsieurtanuki reviewed Feb 17, 2025

View reviewed changes

teolemon added the nutrients label Apr 16, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Nutrient modifiers (Get/Send product APIs) #1042

feat: Nutrient modifiers (Get/Send product APIs) #1042

g123k commented Feb 14, 2025

monsieurtanuki left a comment

monsieurtanuki Feb 14, 2025

g123k Feb 15, 2025

monsieurtanuki Feb 14, 2025

g123k Feb 15, 2025

monsieurtanuki Feb 14, 2025

g123k Feb 15, 2025

monsieurtanuki Feb 14, 2025

monsieurtanuki Feb 14, 2025

g123k commented Feb 15, 2025

monsieurtanuki left a comment

monsieurtanuki Feb 17, 2025

monsieurtanuki Feb 17, 2025

g123k Mar 2, 2025

monsieurtanuki Feb 17, 2025

monsieurtanuki Feb 17, 2025

monsieurtanuki Feb 17, 2025

monsieurtanuki Feb 17, 2025

monsieurtanuki Feb 17, 2025

monsieurtanuki Feb 17, 2025

monsieurtanuki Feb 17, 2025

monsieurtanuki Feb 17, 2025

monsieurtanuki commented Mar 5, 2025

		_valueMap[_getTag(nutrient, perSize)] = value;
		_modifierMap[nutrient.offTag] = modifier;

		final Map<String, NutrientModifier> _modifierMap =
		<String, NutrientModifier>{};

		_valueMap.remove(_getTag(nutrient, perSize));
		_modifierMap[_getModifierTag(nutrient, perSize)] = modifier!;

	nutriments.getModifier(nutrient, perSize),
	nutriments.getModifier(nutrient),

feat: Nutrient modifiers (Get/Send product APIs) #1042

Are you sure you want to change the base?

feat: Nutrient modifiers (Get/Send product APIs) #1042

Conversation

g123k commented Feb 14, 2025

monsieurtanuki left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

g123k commented Feb 15, 2025

monsieurtanuki left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

monsieurtanuki commented Mar 5, 2025