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. > However, if I had an existing set of data, and just tried to upgrade > ofbiz in place, the upgrade itself would have failed, as the entity > engine would not alter the table to add the new primary key field. This is incorrect, OFBiz does not attempt to add new fields to the primary key, it is considered an unsafe operation. A new field is simply added and that field is unconstrained in terms of allowing null values. The developer would have been alerted in the logs during startup and still have the ability to populate the field before manually making the field part of the primary key. > > Even if I did an xml data dump, then reinstalled ofbiz, the xml data > dump would not import, as the data would be missing this new primary > key field. As above, the import in this case would have been successful I believe. I'm not arguing against the deprecation pattern but I do believe there can always be exceptions to our "rules". Regards Scott
smime.p7s
Description: S/MIME cryptographic signature
