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

Clean up operator backend proxy endpoints #23

Merged
merged 9 commits into from
Mar 10, 2022
Merged

Conversation

OlivierChirouze
Copy link
Contributor

Make all proxy endpoints available under /paf-proxy/

* A list of identifiers and a new (unsigned) value for preferences
*/
export interface NewUnsignedPreferences {
unsignedPreferences?: {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IMHO, the semantics and compositions of the Preferences models are confusing. If I translate this line: "new unsigned preferences object that potentially contains unsigned preferences"...

Can you describe how this class helps us, please? It will help to find an alternative.

More generally speaking, in this file, we have many entities that express the Preferences concept: Preferences, PreferenceData, NewUnsignedPreferences, IdsAndPreferences. I have the impression that we generate classes based on the JSON-Schema and so the readability of the models is impacted by how the JSON-Schema works.

Can it be simplified with less class and more semantics? I want to be constructive so here are a few examples but I may not have all the corner cases in mind.

Example 1, the simplest

interface Preferences {
  version: Version;
  data: {
    use_browsing_for_personalization: boolean;
  };
  /** Has a source if the preferences is signed */
  source?: Source;
  /** Weak but explicit relationship to the identifiers. */
  identifiers: Identifier[];
}
// That's it.

Exemple 2, more atomical approach

interface UnsignedPreferences {
  version: Version;
  data: {
    use_browsing_for_personalization: boolean;
  };
}

interface Preferences : UnsignedPreference {
  source: Source;
}

interface PrebidData { 
  preferences: Preferences;
  identifiers: Identifier[];
}

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks.

I get your point and I agree this model is becoming confusing.
I will rework to rename and refine some models.

However, a few comments:

  • The example you commented is a mistake from me: unsignedPreferences shouldn't be optional, I'll fix it, thanks for the heads up.
  • It is complex by nature: here, we want to model an unsigned preferences object, with a list of signed identifiers. I don't have an obvious solution to even name this object. Defining an API often means defining an object per request and response which ends up being quite verbose.
  • I want to avoid using optional attributes like in your suggestion 1. I think strong typing is always safer and more practical than weak typing + validation methods. (I don't want to add a method "if should be signed, then source should exist, otherwise it can be null")
  • I think example 2 that you propose is actually my current approach, except that you mix inheritance and composition, whereas I only used composition. Another difference is that you used names such as PrebidData where I favored explicit (but sometimes pretty long) names such as IdsAndPreferences. I'm not sure PrebidData is more explicit, WDYT?

To sum up, I'll rework these interfaces to make names more explicit and when possible, simplify it, in line with your suggestion 2.

@OlivierChirouze
Copy link
Contributor Author

I've renamed NewUnsignedPreferences which was pretty misleading, fixed the mandatory property, done some small refactoring and used the opportunity to fix an issue that was generating duplicate types (Timestamp1, Timestamp2...)

Note that I haven't used inheritance but only composition, for two reasons:

  1. it is complicated with JSON-schema (see How to do inheritance? json-schema-org/json-schema-spec#348 for example)
  2. I think it's simpler to use composition exclusively rather than mixing both.

I think the current state of the data model is satisfying, again, considering that it is complex by nature.

WDYT?

@RomainLofaso
Copy link
Contributor

I've renamed NewUnsignedPreferences which was pretty misleading, fixed the mandatory property, done some small refactoring and used the opportunity to fix an issue that was generating duplicate types (Timestamp1, Timestamp2...)

Note that I haven't used inheritance but only composition, for two reasons:

1. it is complicated with JSON-schema (see [How to do inheritance? json-schema-org/json-schema-spec#348](https://github.com/json-schema-org/json-schema-spec/issues/348) for example)

2. I think it's simpler to use composition exclusively rather than mixing both.

I think the current state of the data model is satisfying, again, considering that it is complex by nature.

WDYT?

I think that it is more understandable. Thank you.

However, it makes me thinking that using json-schema for the domain was irrelevant. The domain and particularly the models are the most valuable parts of a software because it helps us to understand the business logic. So the expressiness of those entities must be maximized with all the features of the used language. By using json-schema for models, we reduce the possibility offer by the used language and we end up with odd interfaces.

json-schema is meaningful for instance for Data Transfere Objects between two services. The two services relies on the same schema. It is a way to agree on the interface of the communication.

@OlivierChirouze
Copy link
Contributor Author

Honestly, I don't really understand what issues you have with the current model definition.
I think the Preferences, Source, Identifiers and IdsAndPreferences are pretty straight forward, I don't see how they would be better if not generated from JSON-schema.
Can you elaborate?

Or better, can we take it as a separate git issue?

@RomainLofaso
Copy link
Contributor

RomainLofaso commented Mar 8, 2022

Honestly, I don't really understand what issues you have with the current model definition. I think the Preferences, Source, Identifiers and IdsAndPreferences are pretty straight forward, I don't see how they would be better if not generated from JSON-schema. Can you elaborate?

Or better, can we take it as a separate git issue?

I won't create an issue for now. I need to think about it first because I am not sure that it is worth doing the change for a demo. It may be a bit annoying for operator. Briefly, if I reformulate you said that you don't use inheritance because it is not easy with the json-schema. It seems pretty much like the Hammer law. And instead of having optional or state machine or inheritance, we have multiple classes for preferences like Preferences, PreferenceData and UnsignedPreferences. Having a lot of types increase the static analysis but is less simple to understand.

@OlivierChirouze OlivierChirouze merged commit 41ffe7a into main Mar 10, 2022
@OlivierChirouze OlivierChirouze deleted the proxy-cleanup branch March 10, 2022 08:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants