Skip to content
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

Implement the new App uuid system #370

Closed
4 of 5 tasks
teolemon opened this issue Jan 22, 2022 · 17 comments · Fixed by #376
Closed
4 of 5 tasks

Implement the new App uuid system #370

teolemon opened this issue Jan 22, 2022 · 17 comments · Fixed by #376
Assignees

Comments

@teolemon
Copy link
Member

teolemon commented Jan 22, 2022

What

  • Implement the new App uuid system
    • app_name
    • app_version
    • app_uuid
  • Deprecate the equivalent using comment=

For further details and potential questions, see: openfoodfacts/openfoodfacts-server#6319

Part of

@monsieurtanuki
Copy link
Contributor

@teolemon Basically it's about adding 3 optional String parameters to API queries, but will that concern all queries, or just the writes or just the reads?

@monsieurtanuki monsieurtanuki self-assigned this Jan 23, 2022
@M123-dev
Copy link
Member

I think we can edit the UserAgent to be used for that

@teolemon
Copy link
Member Author

  • It's just the WRITE queries at the moment, and you can pass it in the URL, especially for apps that can't send a UserAgent.
  • It's also probably optional, especially since compulsory would create backwards compatibility issues. @stephanegigandet might have a stronger opinion.

@M123-dev
Copy link
Member

M123-dev commented Jan 23, 2022

What apps can't send a UserAgent, the current UserAgent in this sdk is pretty similar, it's only lacking the uuid and is probably send different

https://github.com/openfoodfacts/openfoodfacts-dart/blob/master/lib/model/UserAgent.dart

  final String? name;
  final String? version;
  final String? system;
  final String? url;
  final String? comment;

monsieurtanuki added a commit to monsieurtanuki/openfoodfacts-dart that referenced this issue Jan 23, 2022
New file:
* `SourceConfiguration.dart`: Source Configuration for detection of apps (populates data sources on BE).

Impacted file:
* `openfoodfacts.dart`: added source configuration parameter to `saveProduct`
@stephanegigandet
Copy link
Contributor

I agree with @M123-dev , the info is the same as what we have in the UserAgent class, we can reuse it, so we don't need a separate SourceConfiguration class as proposed by @monsieurtanuki in his PR.

A few points:

  1. Apps should send both the User-Agent header (as we do today) and the extra app_name and app_version parameters (as GET or POST parameters, but they should not be mixed: either send all parameters as GET query string parameters, or all parameters as POST).

The reason we want both is that we can see them in different places. The User-Agent header is easy to find in nginx and Apache logs, while the more structured app_name and app_version are better for the OFF API internals.

  1. The UUID is kind of separate, we can deal with it somewhere else than in the User-Agent class (e.g. when we build the query parameters and add the User info). Maybe AbstractQueryConfiguration

  2. This user-agent and the app_name + app_version should be sent on every request: not only the save product, but also image uploads and crops/selection, and queries to read and search products as well.

  3. In addition, the user specific username, or UUID should be sent for save product, uploading image and image selection (anything that changes a product).

@stephanegigandet
Copy link
Contributor

  1. For read queries / search queries, the SDK should allow apps (and apps users) to easily add (or not add) an user specific OFF username/password. This is to allow future features like accessing your scan history on the website, or other apps.

@monsieurtanuki
Copy link
Contributor

Working on 1. and 3.

monsieurtanuki added a commit to monsieurtanuki/openfoodfacts-dart that referenced this issue Jan 25, 2022
…ies (as parameters)

Impacted files:
* `configuration_test.dart`: added test for user agent; refactored
* `HttpHelper.dart`: new method `addUserAgentParameters`, automatically used for post and multipart requests
* `openfoodfacts.dart`: added `addUserAgentParameters: false` for specific `getUri` calls like post and multipart
* `UriHelper.dart`: new parameter `addUserAgentParameters` for `getUri`
@M123-dev
Copy link
Member

For the uuid, we can probably add it to the user class since it is somewhat related

monsieurtanuki added a commit that referenced this issue Jan 25, 2022
…eters) (#376)

Impacted files:
* `configuration_test.dart`: added test for user agent; refactored
* `HttpHelper.dart`: new method `addUserAgentParameters`, automatically used for post and multipart requests
* `openfoodfacts.dart`: added `addUserAgentParameters: false` for specific `getUri` calls like post and multipart
* `UriHelper.dart`: new parameter `addUserAgentParameters` for `getUri`
@monsieurtanuki
Copy link
Contributor

For the uuid, we can probably add it to the user class since it is somewhat related

Sounds good to me.

But: how do you compute the UUID? I cannot picture the meaning of UUID compared to just the userId (e.g. monsieurtanuki). Should we add the environment, hash them and present the result as a 36-character string? (cf. https://en.wikipedia.org/wiki/Universally_unique_identifier)

@teolemon
Copy link
Member Author

@monsieurtanuki here's what we do in Smoothie (for Robotoff only, I believe)

@monsieurtanuki
Copy link
Contributor

Thank you @teolemon!

As off-dart is not restricted to Android and iOS, we need UUID also for MacOS, Linux, Web and Windows. Something like that:

if (Platform.isAndroid) {
    AndroidDeviceInfo androidInfo = await deviceInfo.androidInfo;
    deviceIdentifier = androidInfo.androidId;
  } else if (Platform.isIOS) {
    IosDeviceInfo iosInfo = await deviceInfo.iosInfo;
    deviceIdentifier = iosInfo.identifierForVendor;
  } else if (kIsWeb) {
    // The web doesnt have a device UID, so use a combination fingerprint as an example
    WebBrowserInfo webInfo = await deviceInfo.webBrowserInfo;
    deviceIdentifier = webInfo.vendor + webInfo.userAgent + webInfo.hardwareConcurrency.toString();
  } else if (Platform.isLinux) {
    LinuxDeviceInfo linuxInfo = await deviceInfo.linuxInfo;
    deviceIdentifier = linuxInfo.machineId;
  } 

Or using platform_device_id:

platfom source example
Windows BIOS UUID 99A4D301-53F5-11CB-8CA0-9CA39A9E1F01
Linux BIOS UUID 32a70060-2a39-437e-88e2-d68e6154de9f
Mac IOPlatformUUID 02662E79-E342-521C-98EA-D4C18B61FEF3
Android AndroidId 9774d56d682e549c
IOS IdentifierForVendor 9C287922-EE26-4501-94B5-DDE6F83E1475

(for web we only get the user agent).

Anyway the general idea is that we (as off-dart) compute the UUID and affect it automatically to the relevant requests, right?
Should we match the "official" UUID format or do we just want something approximately unique, regardless of the string format?

@monsieurtanuki
Copy link
Contributor

There is a couple of problems:

  • off-dart is a dart project, and all the "get device id" packages are flutter packages, not dart packages. They can't be used in dart (as far as I understand)
  • even if it worked, the "get device id" methods are Futures, which has a (manageable) impact on the code, like Uri getUri(...) becoming Future<Uri> getUri(...) async ... with the related strange await getUri(...) calls

Shouldn't we create a static String? uuid field in configuration and let the users set (and store locally) its value, like they do with userAgent? Possibly with helpful comments and helper methods to generate a "decent" UUID?

@jasmeet0817
Copy link
Contributor

jasmeet0817 commented Jan 26, 2022

Sorry for being late to the party.

There is a couple of problems:

  • off-dart is a dart project, and all the "get device id" packages are flutter packages, not dart packages. They can't be used in dart (as far as I understand)
  • even if it worked, the "get device id" methods are Futures, which has a (manageable) impact on the code, like Uri getUri(...) becoming Future<Uri> getUri(...) async ... with the related strange await getUri(...) calls

Agreed, this is not ideal.

Shouldn't we create a static String? uuid field in configuration and let the users set (and store locally) its value,

I think the server should specifically either ask for a device-id or/and (a bunch of specific information like app-name, app-version, username etc) and the server should be responsible for generating the UUID based on that information. Reason being, if you let clients come up with their own mechanics for creating unique strings, two different clients could create the same UUID (which would be unique for them but not across devices). Clients are generally not responsible for creating their own identifiers (unless if they use something globally unique like device-id)

If the server specifically asked for device-id (and app-name+version), that would make this problem much easier, each client would send the device id (and other stuff) to the dart package which would forward it to the server.

@monsieurtanuki
Copy link
Contributor

@jasmeet0817 As I said before, I don't quite understand the purpose of the uuid. If it's something like "device id", we cannot do that in dart but in flutter.

As I suggested, we can provide a dart method to generate UUID. The problem is that we cannot be sure that the user 1. uses this method 2. populates correctly the global uuid field 3. stores and reuses correctly its uuid generated once, instead of generating uuid on the fly when needed.

A solution would be either to convert off-dart to a flutter project (instead of a dart project), or to create a new project called flutter_openfoodfacts that basically embeds (dart) openfoodfacts but with additional features - like an automatic uuid computation from device id.

@jasmeet0817
Copy link
Contributor

hmm I see. Thanks for clarifying. To be honest I would just get the device_id in Smoothie directly, it's already so easy, we don't really need another layer on top.

@teolemon Basic question: Is this really needed in off-dart package ? Can the clients just pass deviceId to the APIs? we can just make it a requirement to always pass it in the APIs we care about.

monsieurtanuki added a commit to monsieurtanuki/openfoodfacts-dart that referenced this issue Feb 2, 2022
Impacted files:
* `configuration_test.dart`: added tests with "uuid"
* `HttpHelper.dart`: included new static `uuid` to parameters
* `OpenFoodAPIConfiguration.dart`: added `static String? uuid`
* `pubspec.yaml`: unrelated upgrade
monsieurtanuki added a commit that referenced this issue Feb 2, 2022
Impacted files:
* `configuration_test.dart`: added tests with "uuid"
* `HttpHelper.dart`: included new static `uuid` to parameters
* `OpenFoodAPIConfiguration.dart`: added `static String? uuid`
* `pubspec.yaml`: unrelated upgrade
@monsieurtanuki
Copy link
Contributor

@teolemon I think we're done here; I guess the part about comment was more server-related.
Feel free to reopen it if relevant.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants