Adding support for generic method to get model using UUID

Description

Adding support to retrieve records using _UU same as done with _ID.

Method can be PO.getPO(String uuid, String trxName)

Environment

None

Activity

Show:
Carlos Ruiz
February 14, 2021, 11:42 AM
Edited

Hi after reviewing the pull request 580, this is what I think:

  • I think is not OK to add constructors using the UUID column, in our current model the UUID column is an accesor, but is not a primary key.

  • Note aside: we have talked about making the UUID the primary key in all tables, after several POCs and analysis I have arrived to the conclusion that making the uuid the primary key would be wrong, and the real way to do this is converting the ID columns to be UUID type, all the indexes and foreign keys are already established with the ID columns, changing the ID to String is feasible, but that change will break everything, so what we have talked here is about making a different “subproject” and provide migration path and tools.

  • So, that comment just to reinforce that I think UUID is not a real primary key and it won’t be because of the effort on setting up the foreign keys, and that makes the approach of UUID based constructors probably wrong.

  • Now, as UUID is an accesor column the get approach seems more correct, and that’s already implemented, but there are good ways to make it easier to get like MTable.getPOByUU implemented in the pull request

  • However, I think the getPOByUU doesn’t require all those changes in PO class (again, the constructors are unnecessary). With the actual code, without this pull request we can achieve the same with this line:

  • And of course it sounds really easy to implement the getPOByUU using exactly this same approach, and there is no need for all the changes in PO:

  • Now, about changing the ModelGenerator to create constructors using uuid, following the same logic, it doesn’t sound good, in the pull request the X_Test is changed to show the new constructor, with this new constructor this can be done:

    but this cannot be done

    unless we add the constructor in the M class, so that change will in practice require us to add the constructor in ALL M classes to be useful (accessing records using the X_ class is not recommended as the before/afterSave are skipped)

  • Also - there is a problem because there can be classes out there that implemented constructors (Properties, String, String), in core we have that problem in MPaymentBatch, and in webstore I found also the same constructor in MClick. So, this is just on core, in plugins it can be possible to find that kind of constructors too.


So, summarizing:

IMO the constructor based UUID is not the right approach, we must implement getter, and that can be implemented without the need of all those changes in PO.

UUID is not a primary key, is an accesor column, we need to think better about moving the ID columns to UUID type - and as we have talked in past with that finally we could get rid of “official IDs”.

Regards,

Carlos Ruiz

Deepak Pansheriya
February 11, 2021, 1:55 PM

Created PR

Heng Sin Low
December 23, 2020, 7:30 AM

I think regardless of which approach, the PO.loadbyuid method is probably a must here corresponding to the existing load by id method.

The problem with adding constructor is not the classes created by model generator but all the M* classes (MOrder, MProduct, etc). You can’t automate that and is a big effort to update all of them.

Deepak Pansheriya
December 23, 2020, 7:14 AM

can we do both approach and in model class generator add constructor for UUID, So that newly generated class has those. At generic places we use less clean approach but in future we start adding proper Constructor in X_ classes.

Heng Sin Low
December 23, 2020, 6:04 AM

IModelFactory.getPO for uuid would be fine.

However, that’s not backward compatible though (all M* class will have to be updated too to incorporate the new constructor).

Alternative backward compatible approach is to create a new PO instance with recordid=0 and add loadByUid(String uid, String trxName) method to PO (similar to existing load(String trxName) method). Less clean API wise but keep backward compatibility.

Fixed

Assignee

Deepak Pansheriya

Reporter

Deepak Pansheriya

Labels

None

Tested By

None

Components

Fix versions

Priority

Major