-
-
Notifications
You must be signed in to change notification settings - Fork 77
feat: 1055 - new experimental class "FlexibleProduct" #1056
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
Closed
Closed
Changes from 11 commits
Commits
Show all changes
16 commits
Select commit
Hold shift + click to select a range
ca19276
feat: 1055 - new experimental class "FlexibleProduct"
monsieurtanuki dbc0caa
Merge branch 'master' into feat/1055
monsieurtanuki 49ae5e5
Merge branch 'master' into feat/1055
monsieurtanuki cd33652
Merge branch 'master' into feat/1055
monsieurtanuki 9ca7936
Merge branch 'master' into feat/1055
monsieurtanuki c83b249
Merge branch 'master' into feat/1055
monsieurtanuki 706246f
Merge branch 'master' into feat/1055
monsieurtanuki d00bb22
Merge branch 'master' into feat/1055
monsieurtanuki b7527af
Merge branch 'master' into feat/1055
monsieurtanuki 42f998b
Merge branch 'master' into feat/1055
monsieurtanuki 0ee58a6
Merge branch 'master' into feat/1055
monsieurtanuki 7b7f046
flexibleFields and schemaVersion
monsieurtanuki c476256
Merge branch 'master' into feat/1055
monsieurtanuki 54a40f5
Merge branch 'master' into feat/1055
monsieurtanuki 42deded
Merge branch 'master' into feat/1055
monsieurtanuki eeaebfb
Merge branch 'master' into feat/1055
monsieurtanuki File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,11 @@ | ||
| import 'package:meta/meta.dart'; | ||
|
|
||
| @experimental | ||
| typedef JsonMap = Map<String, dynamic>; | ||
|
|
||
| @experimental | ||
| abstract class FlexibleMap { | ||
| const FlexibleMap(this.json); | ||
|
|
||
| final JsonMap json; | ||
| } |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,190 @@ | ||
| import 'package:meta/meta.dart'; | ||
|
|
||
| import 'flexible_map.dart'; | ||
| import '../additives.dart'; | ||
| import '../attribute_group.dart'; | ||
| import '../ingredient.dart'; | ||
| import '../knowledge_panels.dart'; | ||
| import '../nutriments.dart'; | ||
| import '../product_image.dart'; | ||
| import '../product_packaging.dart'; | ||
| import '../product_type.dart'; | ||
| import '../../utils/json_helper.dart'; | ||
| import '../../utils/language_helper.dart'; | ||
| import '../../utils/product_fields.dart'; | ||
| import '../../utils/uri_helper.dart'; | ||
|
|
||
| /// Product without predefined structure, relying mainly on the json. | ||
| /// | ||
| /// The typical use case would be "extending" this class in order to control | ||
| /// brand new field values. | ||
| /// The current field list matches what is used in Smoothie. | ||
| @experimental | ||
| class FlexibleProduct extends FlexibleMap { | ||
| FlexibleProduct(super.json); | ||
|
|
||
| FlexibleProduct.fromServer( | ||
| super.json, { | ||
| required final UriProductHelper uriHelper, | ||
| }) { | ||
| json['image_url_base'] = uriHelper.getImageUrlBase(); | ||
| } | ||
|
|
||
| // mandatory for images | ||
| String get imageUrlBase => json['image_url_base']; | ||
|
|
||
| String? get barcode => json['code']; | ||
|
|
||
| ProductType? get productType => ProductType.fromOffTag(json['product_type']); | ||
|
|
||
| int? get schemaVersion => json['schema_version']; | ||
|
|
||
| String? get abbreviatedName => json['abbreviated_product_name']; | ||
|
|
||
| Additives? get additives => | ||
| Additives.additivesFromJson(json['additives_tags'] as List?); | ||
|
|
||
| Iterable<AttributeGroup>? get attributeGroups => | ||
| (json['attribute_groups'] as List<dynamic>?) | ||
| ?.map(AttributeGroup.fromJson); | ||
|
|
||
| String? get brands => json['brands'] as String?; | ||
|
|
||
| String? get categories => json['categories'] as String?; | ||
|
|
||
| Iterable<String>? get categoriesTags => | ||
| (json['categories_tags'] as List<dynamic>?)?.map( | ||
| (e) => e as String, | ||
| ); | ||
|
|
||
| Map<OpenFoodFactsLanguage, List<String>>? get categoriesTagsInLanguages => | ||
| LanguageHelper.fromJsonStringsListMapWithPrefix( | ||
| json, | ||
| inProductField: ProductField.CATEGORIES_TAGS_IN_LANGUAGES, | ||
| ); | ||
|
|
||
| String? get comparedToCategory => json['compared_to_category'] as String?; | ||
|
|
||
| String? get countries => json['countries'] as String?; | ||
|
|
||
| Map<OpenFoodFactsLanguage, List<String>>? get countriesTagsInLanguages => | ||
| LanguageHelper.fromJsonStringsListMapWithPrefix( | ||
| json, | ||
| inProductField: ProductField.COUNTRIES_TAGS_IN_LANGUAGES, | ||
| ); | ||
|
|
||
| String? get embCodes => json['emb_codes'] as String?; | ||
|
|
||
| String? get genericName => json['generic_name'] as String?; | ||
|
|
||
| String? get imageFrontUrl => json['image_front_url'] as String?; | ||
|
|
||
| String? get imageFrontSmallUrl => json['image_front_small_url'] as String?; | ||
|
|
||
| String? get imageIngredientsUrl => json['image_ingredients_url'] as String?; | ||
|
|
||
| String? get imageIngredientsSmallUrl => | ||
| json['image_ingredients_small_url'] as String?; | ||
|
|
||
| String? get imageNutritionUrl => json['image_nutrition_url'] as String?; | ||
|
|
||
| String? get imageNutritionSmallUrl => | ||
| json['image_nutrition_small_url'] as String?; | ||
|
|
||
| String? get imagePackagingUrl => json['image_packaging_url'] as String?; | ||
|
|
||
| String? get imagePackagingSmallUrl => | ||
| json['image_packaging_small_url'] as String?; | ||
|
|
||
| List<ProductImage>? get images => | ||
| JsonHelper.allImagesFromJson(json['images'] as Map?); | ||
|
|
||
| Iterable<Ingredient>? get ingredients => | ||
| (json['ingredients'] as List<dynamic>?) | ||
| ?.map((e) => Ingredient.fromJson(e as Map<String, dynamic>)); | ||
|
|
||
| String? get ingredientsText => json['ingredients_text'] as String?; | ||
|
|
||
| Map<OpenFoodFactsLanguage, String>? get ingredientsTextInLanguages => | ||
| LanguageHelper.fromJsonStringMapWithPrefix( | ||
| json, | ||
| inProductField: ProductField.INGREDIENTS_TEXT_IN_LANGUAGES, | ||
| allProductField: ProductField.INGREDIENTS_TEXT_ALL_LANGUAGES, | ||
| ); | ||
|
|
||
| KnowledgePanels? get knowledgePanels => | ||
| KnowledgePanels.fromJsonHelper(json['knowledge_panels'] as Map?); | ||
|
|
||
| String? get labels => json['labels'] as String?; | ||
|
|
||
| Map<OpenFoodFactsLanguage, List<String>>? get labelsTagsInLanguages => | ||
| LanguageHelper.fromJsonStringsListMapWithPrefix( | ||
| json, | ||
| inProductField: ProductField.LABELS_TAGS_IN_LANGUAGES, | ||
| ); | ||
|
|
||
| OpenFoodFactsLanguage? get lang => | ||
| LanguageHelper.fromJson(json['lang'] as String?); | ||
|
|
||
| DateTime? get lastImage => JsonHelper.timestampToDate(json['last_image_t']); | ||
|
|
||
| Iterable<String>? get lastImageDates => | ||
| (json['last_image_dates_tags'] as List<dynamic>?) | ||
| ?.map((e) => e as String); | ||
|
|
||
| bool? get noNutritionData => | ||
| JsonHelper.checkboxFromJSON(json['no_nutrition_data']); | ||
|
|
||
| String? get nutrimentDataPer => json['nutrition_data_per'] as String?; | ||
|
|
||
| Nutriments? get nutriments => noNutritionData == true | ||
| ? null | ||
| : json['nutriments'] == null | ||
| ? null | ||
| : Nutriments.fromJson(json['nutriments'] as Map<String, dynamic>); | ||
|
|
||
| bool? get obsolete => JsonHelper.checkboxFromJSON(json['obsolete']); | ||
|
|
||
| String? get origins => json['origins'] as String?; | ||
|
|
||
| Map<String, int>? get ownerFields => | ||
| (json['owner_fields'] as Map<String, dynamic>?)?.map( | ||
| (k, e) => MapEntry(k, (e as num).toInt()), | ||
| ); | ||
|
|
||
| Iterable<ProductPackaging>? get packagings => | ||
| (json['packagings'] as List<dynamic>?)?.map(ProductPackaging.fromJson); | ||
|
|
||
| bool? get packagingsComplete => | ||
| JsonHelper.boolFromJSON(json['packagings_complete']); | ||
|
|
||
| Map<OpenFoodFactsLanguage, String>? get packagingTextInLanguages => | ||
| LanguageHelper.fromJsonStringMapWithPrefix( | ||
| json, | ||
| inProductField: ProductField.PACKAGING_TEXT_IN_LANGUAGES, | ||
| allProductField: ProductField.PACKAGING_TEXT_ALL_LANGUAGES, | ||
| ); | ||
|
|
||
| String? get productName => json['product_name'] as String?; | ||
|
|
||
| Map<OpenFoodFactsLanguage, String>? get productNameInLanguages => | ||
| LanguageHelper.fromJsonStringMapWithPrefix( | ||
| json, | ||
| inProductField: ProductField.NAME_IN_LANGUAGES, | ||
| allProductField: ProductField.NAME_ALL_LANGUAGES, | ||
| ); | ||
|
|
||
| String? get quantity => json['quantity'] as String?; | ||
|
|
||
| List<ProductImage>? get selectedImages => | ||
| JsonHelper.selectedImagesFromJson(json['selected_images'] as Map?); | ||
|
|
||
| String? get servingSize => json['serving_size'] as String?; | ||
|
|
||
| Iterable<String>? get statesTags => | ||
| (json['states_tags'] as List<dynamic>?)?.map((e) => e as String); | ||
|
|
||
| String? get stores => json['stores'] as String?; | ||
|
|
||
| String? get website => json['link'] as String?; | ||
| } | ||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,40 @@ | ||
| import 'package:meta/meta.dart'; | ||
|
|
||
| import 'flexible_map.dart'; | ||
| import 'flexible_product.dart'; | ||
| import '../localized_tag.dart'; | ||
| import '../product_result_field_answer.dart'; | ||
| import '../product_result_v3.dart'; | ||
| import '../../utils/uri_helper.dart'; | ||
|
|
||
| /// API answer from a call to /api/v???/product/$barcode, in a flexible manner. | ||
| @experimental | ||
| class FlexibleProductResult extends FlexibleMap { | ||
| FlexibleProductResult( | ||
| super.json, { | ||
| required this.uriHelper, | ||
| }); | ||
|
|
||
| final UriProductHelper uriHelper; | ||
|
|
||
| String? get barcode => json['code'] as String?; | ||
|
|
||
| LocalizedTag? get result => json['result'] == null | ||
| ? null | ||
| : LocalizedTag.fromJson(json['result'] as Map<String, dynamic>); | ||
|
|
||
| String? get status => json['status'] as String?; | ||
|
|
||
| List<ProductResultFieldAnswer>? get errors => | ||
| ProductResultV3.fromJsonListAnswerForField(json['errors']); | ||
|
|
||
| List<ProductResultFieldAnswer>? get warnings => | ||
| ProductResultV3.fromJsonListAnswerForField(json['warnings']); | ||
|
|
||
| FlexibleProduct? get product => json['product'] == null | ||
| ? null | ||
| : FlexibleProduct.fromServer( | ||
| json['product'] as JsonMap, | ||
| uriHelper: uriHelper, | ||
| ); | ||
| } |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,35 @@ | ||
| import 'package:meta/meta.dart'; | ||
|
|
||
| import 'flexible_map.dart'; | ||
| import 'flexible_product.dart'; | ||
| import '../../interface/json_object.dart'; | ||
| import '../../utils/uri_helper.dart'; | ||
|
|
||
| /// API answer from a call product search, in a flexible manner. | ||
| @experimental | ||
| class FlexibleSearchResult extends FlexibleMap { | ||
| FlexibleSearchResult( | ||
| super.json, { | ||
| required this.uriHelper, | ||
| }); | ||
|
|
||
| final UriProductHelper uriHelper; | ||
|
|
||
| int? get page => JsonObject.parseInt(json['page']); | ||
|
|
||
| int? get pageSize => JsonObject.parseInt(json['page_size']); | ||
|
|
||
| int? get count => JsonObject.parseInt(json['count']); | ||
|
|
||
| int? get pageCount => JsonObject.parseInt(json['page_count']); | ||
|
|
||
| int? get skip => JsonObject.parseInt(json['skip']); | ||
|
|
||
| Iterable<FlexibleProduct>? get products => | ||
| (json['products'] as List<dynamic>?)?.map( | ||
| (toElement) => FlexibleProduct.fromServer( | ||
| toElement, | ||
| uriHelper: uriHelper, | ||
| ), | ||
| ); | ||
| } |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Oops, something went wrong.
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But if it doesn't implement this kind of logic I don't understand where this model stands between
Productand storing the product in aMap. Do you already know how this would be handled inFlexibleProductand why it couldn't be implemented inProductdirectly?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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
Productis 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
Productstructure. 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. forbrands- but the data won't be rejected a priori. And extendingFlexibleProducte.g. in Smoothie will make it possible to be flexible about the data.Basically that would be something like
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can implement the same try catch in the Product deserialization logic which I think is what we should do.
This
FlexibleProductapproach 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
Productdirectly.That's my take but it would be nice to get more opinions @g123k @teolemon
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Cool
Correct. In order to provide flexibility to API structure changes.
Correct. So that developers don't have to reinvent the wheel for the 99% for fields that don't change.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 wayFlexibleProductResultcan contain a list of their own type. And same forsearchFlexibleProducts().One concern is if I extend
FlexibleProductto let's say fix the issue withbrandsbeing 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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that would be over-engineering the code:
json, so we can build whatever class on top of that json if neededdartwe can extendFlexibleProductwhenever we want, including in Smoothie, so I can't think of anything we couldn't fix with anextensionIn order to solve this issue forever, the idea would be to deprecate
brandsand to create 2 new getters, approximately coded like that:What do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@PrimaelQuemerais Indeed, we don't need
brandsafter all. WithbrandsAsStringandbrandsAsListwe're good.