Skip to content

[GraphQL] Remove dynamic attributes from ProductInterface #429

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

Merged
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
@@ -0,0 +1,45 @@
# Remove product dynamic attributes from ProductInterface

## Current state

Currently, Magento exposes EAV attributes as part of the product schema as top-level fields of ProductInterface. As consequences:
* The merchant can change the GraphQL from the admin, which makes it hard to cache GraphQL schema.
* Regular product interface overloaded with fields and almost unreadable.
* It is quite often practice to create product programmatically, so the number of attributes could be really excessive with tens thousands of attributes.
* Two Magento instances do not have the same GraphQL schema. As a result, it is near impossible to build a simple client/SDK, which could be easily transferred from one instance to another.
* It is impossible to aggregate the several Magento instances behind the common [BFF](https://docs.microsoft.com/en-us/azure/architecture/patterns/backends-for-frontends).
Copy link
Member

Choose a reason for hiding this comment

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

Extensions can add new attributes on the top level too. Why do we need to hide several Magento instances behind common BFF?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

for instance, if we need to add a common gateway with query cache and want to make this gateway saas solution.


## Proposed solution

We should to remove attributes dynamic attributes,
except system attributes which makes sense for the storefront,
from the product top level and use a [dynamic container](https://github.com/magento/architecture/blob/master/design-documents/graph-ql/custom-attributes-container.md) instead.


```graphql
## ProductInteface with system EAV attributes
type ProductInterface {
uid: ID
name: String
sku: String
attribute_set_id: Int
description: ComplexTextValue
short_description: ComplexTextValue
meta_title: String
meta_keyword: String
meta_description: String
created_at: String
updated_at: String
custom_attributes: [CustomAttribute]
...
}
```

## System EAV attributes that should be deprecated at the storefront
Copy link
Member

Choose a reason for hiding this comment

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

How do you decide which attributes to move to a new container? Why not move for example new_from_date, new_to_date?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Each eav attribute has a property is_system. If the attribute is system this is no way to remove it and very limited capabilities to manage it. that why is safe to assume that this attribute is not dynamic and can be represented as top-level fied.


* `special_price: Float` These fields should not be used a the source of the discount since the discount also could be caused by group price or rule price, which are not reflected in this field.
The same is true for `special_from_date: String` and `special_to_date`.

* `options_container: String` - this field just an exposure of `catalog_product_entity` table field, information about product options could be explicitly retrieved from the corresponding field.
Copy link
Member

Choose a reason for hiding this comment

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

Agree, makes sense to deprecate it.

* `manufacturer: Int` - just a sample attribute from Magento 1.x, we can move it to the custom attributes container.
the same is true for `country_of_manufacture: String`