strapi / rfcs

RFCs for Strapi future changes
70 stars 33 forks source link

Add relation support in cli #6

Open devsecur opened 4 years ago

devsecur commented 4 years ago

Following Issue 4695, I created a pull request for cli relation attribute support

https://github.com/devsecur/rfcs/blob/master/rfcs/0003-cli-generate-relations.md

alexandrebodin commented 4 years ago

Hi @devsecur,

Thank you for this RFC.

I think this feature doesn't even require an RFC as it wouldn't change any of the core features of Strapi nor change any of the core concepts :)

We made some changes to the cli code recently that changes the examples you shared to make the cli simpler. And reworked the content-type-builder code to handle the creation of the models. You might be able to pull that logic outside of the plugin to reuse it for the cli ;)

devsecur commented 4 years ago

Hi @alexandrebodin,

that's awesome. I create a RFC following @lauriejim suggestion in 4695 after created an enhancement ticket on the main repository.

Which branch in the main repository contains the most recent changes of the cli? I would have a look at it and port the logic of the content-type-builder plugin if necessary to enhance the latest state of the cli - did i get you right?

Would I then make a pull on the main repository with an enhanced cli if successful?

lauriejim commented 4 years ago

I suggest that to define the attribute format attribute_name:attribute_type currently. But what would it be for relations?

devsecur commented 4 years ago

For relations, we have a name, a type, the related api and the form of relation (many-to-many, one-to-many,...). This seems to be more complex.

Following the notation:

attibute_name:attribute_type:form:related

On the other hand, the cli has options. These could be used for attributes:

attribute_name:attribute_type --form form --related name

I have a bias for the second

alexandrebodin commented 4 years ago

Using that complexe of a format is going to get really difficult to understand.

We might be better of just providing a --file option to pass a full schema. This would meet the need for automation and be a lot easier to use.

For a user wanting to do it manually I would go for a step by step prompt to be a lot more reliable and user friendly

devsecur commented 4 years ago

As the main propose for my request was automation, I created a json file containing the scheme and a script parsing this file and executing strapi cli. The file was the following:

{
  "api": {
    "person": {
      "name": {
        "type": "string"
        }
    },
    "location": {
      "owner": {
        "type": "relation",
        "target": "person",
        "form": "onetoone"
      }
    }
  },
  "controller": {},
  "model": {},
  "service": {},
  "policy": {},
  "plugin": {},
  "install": {},
  "uninstall": {}
}

As i didn't needed the other commands right now, I only used there the options following the cli documentation.

devsecur commented 4 years ago

To be more complete, maybe the attributes should be in an own "attribute" node within the api element and also add plugin and tpl

devsecur commented 4 years ago

For a user wanting to do it manually I would go for a step by step prompt to be a lot more reliable and user friendly

The second form

attribute_name:attribute_type --form form --related name

could trigger the step by step prompt, if the options are missing to be more reliable and user friendly but automatable at the same time

alexandrebodin commented 4 years ago

Running a prompt if parameters are missing will create problem for automation. We should run by default in interactive mode or pass a file. or an encoded json maybe.

Using cli params like --form or --related will be a problem if you want to create multiple relations. I really don't think it makes sense to allow for such a complexe cli when we can just load from a file.

We could have 3 ways to run the generator

We could even imagine feeding it as stdin like

cat model.json | strapi generate:model

This could be really useful for automation

devsecur commented 4 years ago

Yeah sounds great for me!

alexandrebodin commented 4 years ago

Would you be willing to update the RFC to reflect the proposed changes. We need to figure out the way we can declare relations. You should look at how the content-type-buidler works to see that we delcare relation as follows:

Example model

{
  "attributes": {
    "category": {
      "nature": "oneToOne",
      "target": "application::category.category",
      "relatedAttribute": "restaurant"
    }
  }
}
devsecur commented 4 years ago

Would you be willing to update the RFC to reflect the proposed changes. We need to figure out the way we can declare relations. You should look at how the content-type-buidler works to see that we delcare relation as follows:

Sure @alexandrebodin , I tried to update the rfc with the consense on file and interactive cli. I also included typed following content-type-builder types and relations

But i'm not sure, if I understand the content-type-buidler plugin and the need of definition in rfc right - Is my adjustment sufficient? I didn't understand the type "component" and "dynamiczone"

I didn't understand some attributes in relations such as "columnName" and "targetColumnName" as well. Isn't the reverse attribute in the related api generated automatically following a naming convention? Do I have to name the relatedAttribute only in the relating api or do I have to create the attribute first in the related api and then relate the attribute in the relating api? Sorry for the confusion.

derrickmehaffy commented 4 years ago

Would you be willing to update the RFC to reflect the proposed changes. We need to figure out the way we can declare relations. You should look at how the content-type-buidler works to see that we delcare relation as follows:

Example model

{
  "attributes": {
    "category": {
      "nature": "oneToOne",
      "target": "application::category.category",
      "relatedAttribute": "restaurant"
    }
  }
}

@alexandrebodin I would love to see this schema change, it would make the definitions of relations so much more clear. Although the actual migration process for existing users will be.... painful. If possible we should also consider an automated script to handle this.

strapi-cla commented 3 years ago

CLA assistant check
All committers have signed the CLA.

Eggwise commented 3 years ago

what is the status on this ?