Scott Gray wrote: > On 18/03/2010, at 11:48 AM, Adam Heath wrote: > >> Scott Gray wrote: >>> On 18/03/2010, at 11:04 AM, Adam Heath wrote: >>> >>>> Scott Gray wrote: >>>>> On 18/03/2010, at 10:50 AM, Adam Heath wrote: >>>>> >>>>>> Jacopo Cappellato wrote: >>>>>>> Adam, >>>>>>> >>>>>>> this is going to be a long thread and I don't want it to be. >>>>>>> I already tried to explain why I decided to change the primary key of >>>>>>> OrderShipment without following the deprecation mechanism (that I think >>>>>>> it is a very good best practice) in this specific situation. >>>>>>> I perfectly know when and how the shipment plans (i.e. the feature for >>>>>>> which the OrderShipment entity was introduced) were implemented: I know >>>>>>> because I contributed it (and the OrderShipment entity of course). >>>>>>> I also know that it was only partially implemented and so I would be >>>>>>> very surprised (especially because no one over the years ever asked or >>>>>>> reported bugs etc.. about it - apart from me that was aware of its >>>>>>> existence) if it is really used in production by someone. This is >>>>>>> simply to explain that I was not irresponsible when I did it. >>>>>>> >>>>>>> However you are right if you say that I can't be completely sure if >>>>>>> someone somewhere is using the entity. Also, lately the community seems >>>>>>> to be more inclined to transform "best practices" (to be applied "cum >>>>>>> granu salis") into "policies" to be applied blindly. >>>>>>> In some way this is good because it requires less skills and creativity >>>>>>> (but more pragmatism) and also less skilled persons can become good >>>>>>> reviewers just by following written rules. >>>>>>> This is probably less interesting but maybe necessary given the grow of >>>>>>> the community. >>>>>>> >>>>>>> So ok, I am convinced now, we have to deprecate that entity etc. >>>>>>> >>>>>>> What does your book of policies says about this: is it something I >>>>>>> *have* to do as a punishment for this bad commit or will you take care >>>>>>> of it? >>>>>> If I implied that it had to be you to do it, I apoligize. I didn't >>>>>> mean it to sound that way. >>>>>> >>>>>> I was trying to discuss with you about the reasons behind this change, >>>>>> so that we could all agree as to what the reasons and use cases were. >>>>>> >>>>>> Now that it seems we are on the same page, we can go about fixing this >>>>>> as a team. I'll admit, however, that I have never gone thru an entity >>>>>> deprecation exercise. And OrderShipment is a very low-level entity, >>>>>> that has tons of references, so doing this right could be very >>>>>> complex. The reason why I was so heated was that I understood this >>>>>> would be a lot of work, and I was trying to see if there is a better >>>>>> way than changing the primary key. >>>>>> >>>>>> The reason the original committer is generally the best one to fix >>>>>> their original commit, is because that original committer knows the >>>>>> most about what they were trying to do. That original person knows >>>>>> their own code they wrote, and probably has an understand of the rest >>>>>> of the files that they were modifying. >>>>>> >>>>>> However, due to the scope of this deprecation, we need the rest of the >>>>>> community to help out. Unless we decide to revert this primary key >>>>>> change, and due this a different way. >>>>>> >>>>>>> PS: BTW, if no one will implement the deprecation pattern in the next >>>>>>> few days I will do that, no problem. >>>>>> There's no big hurry. My previously mentioned importer already had >>>>>> the shipGroupSeqId available(it was hard-coded to 00001, as it's >>>>>> somewhat simple), so it wasn't a big change for me personally. >>>>> I'm yet to form an opinion on whether the approach Jacopo took was a >>>>> valid one, I've been looking at the code and I don't yet understand what >>>>> problems this change may have caused for downstream users. >>>>> Adam, would you mind explaining in more detail how the change has >>>>> effected you negatively? >>>> A new field was added to the OrderShipment entity, and that field was >>>> made a primary key. >>>> >>>> My importer, which started from an empty database, thru an error, >>>> because this field was not set, and null is not allowed. As I said, >>>> the fix for me was simple, as this particular job hasn't gone live >>>> yet, so I don't have to worry so much about migration. >>> I would argue that in this case the early failure was a good thing. If the >>> deprecation pattern had been followed it's possible you wouldn't have >>> noticed until much later and then would have had to deal with updating your >>> importer as well as migrating the data from the old table to the new. >> You don't understand how deprecation works. > > That is a strong statement but the teacher has spoken, I shall go and face > the corner now. > >> The entity is renamed. So there would still be the failure in my >> importer, as it tried to update an entity that doesn't exist. > > I assumed your importer was acting on the database directly, my bad. > >> Additionally, if someone upgrading their data does an xml dump, the >> old data will be dumped with the entity named OrderShipment. After >> installing the newer ofbiz, that identiy will also not exist, so the >> xml import will fail as well, when it can't find the OrderShipment >> entity(which was renamed to OldOrderShipment). > > > Since I don't understand how deprecation works I guess I'm also failing to > understand how what you describe would be better than what would happen right > now. > > Once again, I'm not against the deprecation pattern, I'm just struggling to > see how it would have resulted in something better for the downstream user > (but because I don't understand how deprecation works this probably requires > more explaining that it would for someone who is more enlightened).
Read this: http://cwiki.apache.org/confluence/display/OFBTECH/General+Entity+Overview the part about deprecated entities. Before deprecation: <OrderShipment />, has a table named "order_shipment" in the database. After deprecation: <OldOrderShipment tableName="order_shipment"/> <OrderShip/> <service name="convertOldOrderShipmentToOrderShip"/> <!-- all other places that reference/deal with OrderShipment are changed to deal with OrderShip --> During upgrade, the new OrderShip entity will have a table created in the database, called "order_ship". This table will be empty at this point. The existing tables will not be changed at all. This is because the OldOrderShipment entity is mapped to the "order_shipment". Any other tables that also still exist and reference "order_shipment" in the database will continue to function correctly. Actually, I just noticed a problem with the deprecation policy. Any existing entities that currently reference OrderShipment, by way of <relation>, can't have those relations dropped. Those should be changed to "OldFooRelation", and the fk-name set appropriately, so that the referential links in the database will be kept and not destroyed.
