Platform-OS / platformos-documentation

Source files for the official platformOS Documentation.
https://documentation.platformos.com
Other
13 stars 31 forks source link

Rename `models` and `model_schema` #1297

Closed Slashek closed 3 years ago

Slashek commented 3 years ago

We reached the point when Model is flexible enough to replace both Transactable (a deprecated long time ago) and UserProfile. The naming, however, is still confusing for most people, especially with DB background. We would like to fix the naming and use intuitive nouns instead of having to explain it in the documentation.

We would like to change Model Schema to Table and Model to Record. This means that for example instead of having model_create we will have now record_create, instead of admin_model_schemas we will have admin_tables etc. This should make platformOS more intuitive to use. We would like to keep property as it is, as we didn't hear negative feedback about that one.

[UPDATED] - Some links from the comments below to PRs which update old names:

examples - mdyd-dev/platformos-examples#90 template - mdyd-dev/product-marketplace-template#93

Developer-DNM commented 3 years ago

What do you think @onecreative ? ๐Ÿ‘

onecreative commented 3 years ago

As much as I fought for models, I think you are correct. The system has simplified enough that we donโ€™t need special names for this.

Agreed. This is actually a sign that things are going in the right direction.

pavelloz commented 3 years ago

Lets provide codemod ( https://github.com/facebook/codemod - or maybe just simple JS script) for developers convenience to migrate from old to new endpoint names. It should speed up adoption, ergo, speed up deprecation date.

richxrich commented 3 years ago

We used database, table, row, column when we build OOC Tools for the module marketplace. While it might not be technically the right name, anyone that can use Excel will understand the terms and in turn how to use our software.

codemod would only solve for us about 50% for this level of refactoring. Please roll this out slowly, give a big heads up, keep backward compatibility for time period (if possible and feasible). Don't just say "it's live, let us know if anything breaks", because most of the time it breaks something for us.

pavelloz commented 3 years ago

It wont break anything, old endpoints will stay, new will appear.

piotrze commented 3 years ago

@Developer-DNM Prisma use model instead of table https://www.prisma.io/docs/concepts/components/prisma-client/crud#findone

pavelloz commented 3 years ago

^ better link to show it: https://www.prisma.io/docs/concepts/components/prisma-schema/data-model/

Slashek commented 3 years ago

First PR was merged - GraphQL models query was deprecated, and a new records query has been created. Full migration path:

Migration hints:

Example before

query {
      programmers: models(per_page: 10, filter: { model_schema_name: { value_in: ["programmer"] } }) { results { id } }
      places: models(per_page: 10, filter: { model_schema_name: { value_in: ["place"] } }) { results { id } }

      companies: models(per_page: 10 filter: { model_schema_name: { value: "company" } }) {
        results {
          id
          programmers: related_models(join_on_property: "id" foreign_property: "company_id" model_schema_name: "programmer") { id created_at updated_at }
          specific_programmer: related_model(join_on_property: "id" foreign_property: "company_id" model_schema_name: "programmer" filter: { id: { value: "1" }}) { id created_at updated_at }
          last_programmer: related_model(join_on_property: "id" foreign_property: "company_id" model_schema_name: "programmer") { id created_at updated_at }
          places: related_models(join_on_property: "id" foreign_property: "company_id" model_schema_name: "place") {
            id
          }
        }
      }
    }

Example after:

query {
      programmers: records(per_page: 10, filter: { table: { value_in: ["programmer"] } }) { results { id } }
      places: records(per_page: 10, filter: { table: { value_in: ["place"] } }) { results { id } }

      companies: records(per_page: 10 filter: { table: { value: "company" } }) {
        results {
          id
          programmers: related_records(join_on_property: "id" foreign_property: "company_id" table: "programmer") { id created_at updated_at }
          specific_programmer: related_record(join_on_property: "id" foreign_property: "company_id" table: "programmer" filter: { id: { value: "1" }}) { id created_at updated_at }
          last_programmer: related_record(join_on_property: "id" foreign_property: "company_id" table: "programmer") { id created_at updated_at }
          places: related_records(join_on_property: "id" foreign_property: "company_id" table: "place") {
            id
          }
        }
      }

More examples can be found in our open source repositories:

pavelloz commented 3 years ago

Migration path for *nix systems:

find . -type f -name "*.graphql" -print0 | xargs -0 sed -i 's/model_schema_name/table/'
find . -type f -name "*.graphql" -print0 | xargs -0 sed -i 's/model(/record(/'
find . -type f -name "*.graphql" -print0 | xargs -0 sed -i 's/models(/records(/'

Example migration commit: https://github.com/mdyd-dev/product-marketplace-template/pull/93/commits/1f790a46bb885bb18f9e99b7bcfa251a7d5342f5

Slashek commented 3 years ago

Migration path for *nix systems:

find . -type f -name "*.graphql" -print0 | xargs -0 sed -i 's/model_schema_name/table/'
find . -type f -name "*.graphql" -print0 | xargs -0 sed -i 's/model(/record(/'
find . -type f -name "*.graphql" -print0 | xargs -0 sed -i 's/models(/records(/'

Example migration commit: mdyd-dev/product-marketplace-template@1f790a4

Just please note that views would need to be updated accordingly, because {% graphql res = 'get_models' %} will not have res.records, any the code relies on res.models

darylbarnes commented 3 years ago

@Slashek I don't think Table is a good replacement for Model Schemas. I would instead go with Model because while it does in fact map to a "table" in a database, a "table" implies a specific representational state of the data you're modeling which can cause conceptional problems now and in the future when for example the data you're modeling is better represented by a chart rather than a table. As @piotrze and @pavelloz pointed out, Prisma went with that name as well and I would suspect for much the same reasons.

There's also the fact that since a Record now maps to a "row" in the "table" you need the documentation to be able to stay on the same level of a single abstraction from the spreadsheet concept that anyone can understand. It's a nice middle ground in my opinion.

onecreative commented 3 years ago

@darylbarnes models were good when we had different types of db tables in the system. When that was the case, we needed to name them according to their concept. But the holy grail of simplicity is when you don't need to give something a conceptual name, but can simply call it what it is. @Slashek is right. We no longer have other table concepts, with the exception of the user table. So why not take the win and call it what it is?

darylbarnes commented 3 years ago

@onecreative simplicity is good. The problem with the original design of the system was that Models were actually mapped to "rows" instead of "tables" in the database. And Model Schemas were mapped to the "tables".

But now that Records are correctly being mapped to "rows", that frees up Models to be mapped to "tables" as it should have been all along.

The current proposal for Tables actually introduces complexity because now you have a concept about "tables" and "rows" in a spreadsheet entering into the design of the system but you're only implementing it halfway. You can't use Table without also using Row and it be intuitive. I've used @richxrich database module and the simplicity is there because he only refers to tables and rows. But that's great because he's coding a system for a non technical end user experience and he exposes the most user friendly names to them of "tables", "rows", and "columns". On the back end that allows module builders to cleanly map the front end experience to the back end.

You don't want the back end concepts to spill into the front end unless you do it fully and don't ever want to change your api. As soon as they are out of sync it introduces complexity... you see tables and so you intuitively think a listing on the table is a row but then you get completely side swiped by the fact that it is not a "row" but a "record". No consistency means it's counter-intuitive. You can't have your foot in 2 different concepts. You need to use either tables and rows on the backend or models and records. Choose one or the other.

I strongly suggest models and records on the backend/process api so that the frontend/systems api and end user documentation can refer to the fact that models and records map to "tables" and "rows" in a spreadsheet. That keeps the relations clean and user friendly and makes for documentation that is easy to explain and understand. @diana-lakatos what do you think?

darylbarnes commented 3 years ago

We used database, table, row, column when we build OOC Tools for the module marketplace. While it might not be technically the right name, anyone that can use Excel will understand the terms and in turn how to use our software.

And that's exactly how it should be. Records and models are concepts module builders want on the backend api because it is technically correct. Then we can choose whether or not to expose a more user friendly terminology to our end users by mapping to those names.

mattwalter91 commented 3 years ago

models/model would only work if there's zero support for backwards compatibility. The name is still in use.

onecreative commented 3 years ago

Good point, @mattwalter91. The point is moot because they screwed up the naming when they originally did model_schemas and models. If they had done models and records, it would have been a different story. But that opportunity is passed. The real choice is between model_schemas, which is redundant and has always driven me nuts, and tables.

darylbarnes commented 3 years ago

models/model would only work if there's zero support for backwards compatibility. The name is still in use.

True. I don't mind a clean break in this case though because I think it's very important to get this right. I would also like to see the platform end up with a directory structure with models, views, and controllers. We already have views. We now have the opportunity to have models. Tables and model_schemas just don't fit. I say just leave model_schemas alone until the old models goes from deprecated to no longer supported. Accelerate that schedule. Then implement models the way it can and should be done now.

onecreative commented 3 years ago

True. I don't mind a clean break in this case though because I think it's very important to get this right.

I think this is bound to be a losing battle. If there is any single area that requires backwards compatibility, this may be it. I can see some partners leaving over that kind of a break. I can see CBO considering other options over that kind of a break.

darylbarnes commented 3 years ago

Well you and I talked about the need to fix this way back before people started building a lot of things on it because of how important naming and structure is and how hard it is to change afterwards. There is a reason you fought so hard for models and what a valiant fight it was... you lost an early battle but don't give up because the war isn't over! ๐Ÿ˜

It is what it is at this point. As long as there is a plan and a schedule, I would hope any partners would understand the importance of getting the foundations right. The fact that this is the area needing the most backwards compatibility just goes to prove how foundational it is. After all, new partners coming on board will be judging their options as well, new developers will be learning the system, and you don't want this to be a stumbling block. I believe platformOS has a brighter future ahead than where it is now. This might be better discussed in a private slack channel or something.

onecreative commented 3 years ago

I think this is the ideal place to discuss these things. We just need to pick subjects where there is still a chance to make a change. I think this one is as good as closed.

Slashek commented 3 years ago

It's awesome to see all the contributions and discussions - thanks! Since day 1 we were explaining our concepts as "think of Model as DB Record and ModelSchema as DB Table" and it worked quite nicely, that's why we decided to stick with this naming. However, instead of putting tables definitions to tables/ directory, we went with the schema directory - because we want to be able to add later some new concepts, which are not necessarily tables, but also belong to schema - like for example concept of "ElasticSearch Views" - something we have talked a lot in the past. We believe this naming will fix issues with the current naming and will allow us to add new features without having to rename things again.

Since this issue was created, we have managed to make our community site https://community.platformos.com/ up and running, happy to continue the discussion there! Closing the ticket, we will be updating the documentation now.