entity primary key change -> BAD: Re: svn commit: r895250 - in /ofbiz/trunk/applications: order/entitydef/ product/entitydef/ product/script/org/ofbiz/shipment/issuance/ product/script/org/ofbiz/shipment/receipt/ product/script/org/ofbiz/shipment/shipment/ product/servicedef/ product/webap...

[Ean Schuessler]

Jacopo Cappellato wrote:
> What I find interesting is that your OrderShipment entity was not empty.
>  
Why is it unusual for data to be in that table? Sales orders on trunk
are generating data in that table. Numerous services and screens use the
entity.

--
Ean Schuessler, CTO
[hidden email]
214-720-0700 x 315
Brainfood, Inc.
http://www.brainfood.com

[Scott Gray-2]

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).

[Adam Heath-2]

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.

[Scott Gray-2]

On 18/03/2010, at 12:36 PM, Adam Heath wrote:

> 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.

I am aware of the document and understand the concepts put forward.

What I am asking you is, how in this case would following the deprecation policy have resulted in a better downstream user experience?

My current understanding was that Jacopo chose the approach he did because it was a simpler one and resulted in a cleaner data model (i.e. no Old* entity left lying around until the end of time).  What I don't understand is how downstream users will suffer extra pain because the deprecation guideline wasn't followed in this case.

[Jacopo Cappellato-4]

Hi Ean,

On Mar 18, 2010, at 7:05 PM, Ean Schuessler wrote:

> Jacopo Cappellato wrote:
>> What I find interesting is that your OrderShipment entity was not empty.
>>  
> Why is it unusual for data to be in that table? Sales orders on trunk are generating data in that table. Numerous services and screens use the entity.

OrderShipment is used now, because I refactored the code in that commit; it was not used before that (apart from the "shipment plan" screen I mentioned, but as I said I doubt companies could use it in production).

Jacopo

>
> --
> Ean Schuessler, CTO
> [hidden email]
> 214-720-0700 x 315
> Brainfood, Inc.
> http://www.brainfood.com
>

[Jacopo Cappellato-4]

On Mar 18, 2010, at 6:30 PM, Adam Heath wrote:

> Jacopo Cappellato wrote:
>> On Mar 18, 2010, at 6:04 PM, Adam Heath wrote:
>>
>>> My importer, which started from an empty database, thru an error,
>>> because this field was not set, and null is not allowed.
>>
>> What I find interesting is that your OrderShipment entity was not empty.
>
> Because my importer was creating completed/approved orders, and that
> is how shipments are done.  To get them to show in the backend
> ordermgr, you have to create OrderShipment entities.

Before or after my commit?

Jacopo

>
> Or, do you mean that no items were listed in the shipment?
>
> Before this change, I could import old orders into a completed state.
> Approved orders, one that exist in the old system, but haven't been
> fully shipped(due to inventory not yet being available, or they just
> haven't been fully processed), also worked.  I could do a quick ship
> entire order, and everything would function as it should.
>
> It may be that your change isn't really needed.  But I will admit I
> don't have a complete knowledge of everything.
>

[Jacopo Cappellato-4]

On Mar 18, 2010, at 6:37 PM, Adam Heath wrote:

>>
>> It may be that your change isn't really needed.  But I will admit I
>> don't have a complete knowledge of everything.
>
> OrderItemShipGroupAssoc?

Is it a question for me?

Jacopo

[Adam Heath-2]

Jacopo Cappellato wrote:

> Hi Ean,
>
> On Mar 18, 2010, at 7:05 PM, Ean Schuessler wrote:
>
>> Jacopo Cappellato wrote:
>>> What I find interesting is that your OrderShipment entity was not empty.
>>>  
>> Why is it unusual for data to be in that table? Sales orders on trunk are generating data in that table. Numerous services and screens use the entity.
>
> OrderShipment is used now, because I refactored the code in that commit; it was not used before that (apart from the "shipment plan" screen I mentioned, but as I said I doubt companies could use it in production).

That's not correct, it was used at least by Sept 13, which is the
version of ofbiz that my importer was originally run on, and that I
used to figure out what I had to do.  I used ecommerce to see how
orders were created.

[Adam Heath-2]

Jacopo Cappellato wrote:

> On Mar 18, 2010, at 6:30 PM, Adam Heath wrote:
>
>> Jacopo Cappellato wrote:
>>> On Mar 18, 2010, at 6:04 PM, Adam Heath wrote:
>>>
>>>> My importer, which started from an empty database, thru an error,
>>>> because this field was not set, and null is not allowed.
>>> What I find interesting is that your OrderShipment entity was not empty.
>> Because my importer was creating completed/approved orders, and that
>> is how shipments are done.  To get them to show in the backend
>> ordermgr, you have to create OrderShipment entities.
>
> Before or after my commit?

Before your commit, it worked, and OrderShipment entities were in use.

[Adam Heath-2]

Jacopo Cappellato wrote:
> On Mar 18, 2010, at 6:37 PM, Adam Heath wrote:
>
>>> It may be that your change isn't really needed.  But I will admit I
>>> don't have a complete knowledge of everything.
>> OrderItemShipGroupAssoc?
>
> Is it a question for me?

Maybe that's the entity that is actually used, meaning you wouldn't
need to do that change you did.

[Marc Morin]

I loath to enter into this discussion, but feel that I need to put in a bit of experience that I have observed when dealing with databases and migration issues.

There are a couple of truisms that I have come to accept, grudgingly:

1) Database schemas will change over time.
2) Database schema changes are a pain to deal with.  Migration issue are difficult.
3) Unlike changes in code, that have no "history" to them (other than an interface specification to external api), database changes, have a long time horizon.  Decisions that were rational that lead to a schema design at the time, but new use cases, etc.. result in re-factoring or enhancing cause you to continually deal with the implications of this "old" design.
4) It's impossible to satisfy all schema API revision constraints at the same time.
5) You need to have a formal way to manage migration of data in your framework.
6) Only support the newest schema, don't leave legacy artifacts around in schema, tends to increase duplication, increased confusion, and increased "code entropy".
7) Avoid schema as an API point in your system if at all possible... can really tie your hands from a development point of view.  Means other systems will need to be updated when merging...

So, Ofbiz doesn't really have any formal migration process. checkdb() adds missing stuff, but is incomplete in some changes (column type changes, index changes, pkey changes, drops, null/not null changes), but more importantly, the data migration framework is meant to be outside, and manual... leading to problems, inconstancies, and more importantly, difficulties.

The "rename" the old entity, create a migration service, then manually run it, is a "weak" migration framework.

I have had success when trying to keep a tighter lid on things:

1- automatically create .sql migration files representing the differences in schemas. (rev<n> - rev<n-1> differences).
2- automatically add insert/update/delete commands representing the differences in seed into these files.
3- manually edit these files, or generate new ones to represent migration of data, where the "simple" cases are not handled, such as moving data from one entity, to another, etc...
4- these sql migration commands become formally part of the codebase.  Migration scripts automatically run on older schema versions (trick is the detect what version the schema actually is.... can't rely on version number, etc... too easy to be inaccurate, and run all "needed" migration commands).
5- Only supporting the "current" schema in the code base.  This means that as the schema evolves, code that isn't in the project will need to be upgraded when merging in these changes.

Obviously the above series of steps are meant to move all "legacy" databases through an upgrade process to the current revision, such that they should be indistinguishable from being created from a newly minted schema.

I have done this type of support in my past life, with another project, other than ofbiz and it helped tame the affects of schema changes.  We also have developed this approach at Emforium to manage the upgrade of our customer's ofbiz instances.  It has been VERY DIFFICULT to keep these instances consistent with one another, as the schema has changed over time.... I won't say that this is 100% nailed down, but we are on our way.

So, I'd be interested in discussing how we can add a more formal migration declaration into ofbiz, with forward and backward migrations formally put into place (like ruby on rails, etc...).

We can talk about using our developed framework for this, but there are limitations; such as .sql files are not likely portable, and certainly can't be easily split up between datasources.

We've also generated a number of self tests, to test that a db at rev<n-1> can be upgraded to rev<n> with the seed to be identical, this is CRUCIAL to ensure, that the migration chain is unbroken...


Marc

[Tim Ruppert]

Thanks for the input Marc - this is definitely something that the project needs and your contributions in this arena would definitely make a huge impact.

Cheers,
Ruppert

On Mar 18, 2010, at 7:18 PM, Marc Morin wrote:

> I loath to enter into this discussion, but feel that I need to put in a bit of experience that I have observed when dealing with databases and migration issues.
>
> There are a couple of truisms that I have come to accept, grudgingly:
>
> 1) Database schemas will change over time.
> 2) Database schema changes are a pain to deal with.  Migration issue are difficult.
> 3) Unlike changes in code, that have no "history" to them (other than an interface specification to external api), database changes, have a long time horizon.  Decisions that were rational that lead to a schema design at the time, but new use cases, etc.. result in re-factoring or enhancing cause you to continually deal with the implications of this "old" design.
> 4) It's impossible to satisfy all schema API revision constraints at the same time.
> 5) You need to have a formal way to manage migration of data in your framework.
> 6) Only support the newest schema, don't leave legacy artifacts around in schema, tends to increase duplication, increased confusion, and increased "code entropy".
> 7) Avoid schema as an API point in your system if at all possible... can really tie your hands from a development point of view.  Means other systems will need to be updated when merging...
>
> So, Ofbiz doesn't really have any formal migration process. checkdb() adds missing stuff, but is incomplete in some changes (column type changes, index changes, pkey changes, drops, null/not null changes), but more importantly, the data migration framework is meant to be outside, and manual... leading to problems, inconstancies, and more importantly, difficulties.
>
> The "rename" the old entity, create a migration service, then manually run it, is a "weak" migration framework.
>
> I have had success when trying to keep a tighter lid on things:
>
> 1- automatically create .sql migration files representing the differences in schemas. (rev<n> - rev<n-1> differences).
> 2- automatically add insert/update/delete commands representing the differences in seed into these files.
> 3- manually edit these files, or generate new ones to represent migration of data, where the "simple" cases are not handled, such as moving data from one entity, to another, etc...
> 4- these sql migration commands become formally part of the codebase.  Migration scripts automatically run on older schema versions (trick is the detect what version the schema actually is.... can't rely on version number, etc... too easy to be inaccurate, and run all "needed" migration commands).
> 5- Only supporting the "current" schema in the code base.  This means that as the schema evolves, code that isn't in the project will need to be upgraded when merging in these changes.
>
> Obviously the above series of steps are meant to move all "legacy" databases through an upgrade process to the current revision, such that they should be indistinguishable from being created from a newly minted schema.
>
> I have done this type of support in my past life, with another project, other than ofbiz and it helped tame the affects of schema changes.  We also have developed this approach at Emforium to manage the upgrade of our customer's ofbiz instances.  It has been VERY DIFFICULT to keep these instances consistent with one another, as the schema has changed over time.... I won't say that this is 100% nailed down, but we are on our way.
>
> So, I'd be interested in discussing how we can add a more formal migration declaration into ofbiz, with forward and backward migrations formally put into place (like ruby on rails, etc...).
>
> We can talk about using our developed framework for this, but there are limitations; such as .sql files are not likely portable, and certainly can't be easily split up between datasources.
>
> We've also generated a number of self tests, to test that a db at rev<n-1> can be upgraded to rev<n> with the seed to be identical, this is CRUCIAL to ensure, that the migration chain is unbroken...
>
>
> Marc
>
>

[Adam Heath-2]

Marc Morin wrote:

> I loath to enter into this discussion, but feel that I need to put in a bit of experience that I have observed when dealing with databases and migration issues.
>
> There are a couple of truisms that I have come to accept, grudgingly:
>
> 1) Database schemas will change over time.
> 2) Database schema changes are a pain to deal with.  Migration issue are difficult.
> 3) Unlike changes in code, that have no "history" to them (other than an interface specification to external api), database changes, have a long time horizon.  Decisions that were rational that lead to a schema design at the time, but new use cases, etc.. result in re-factoring or enhancing cause you to continually deal with the implications of this "old" design.
> 4) It's impossible to satisfy all schema API revision constraints at the same time.
> 5) You need to have a formal way to manage migration of data in your framework.
> 6) Only support the newest schema, don't leave legacy artifacts around in schema, tends to increase duplication, increased confusion, and increased "code entropy".
> 7) Avoid schema as an API point in your system if at all possible... can really tie your hands from a development point of view.  Means other systems will need to be updated when merging...
>
> So, Ofbiz doesn't really have any formal migration process. checkdb() adds missing stuff, but is incomplete in some changes (column type changes, index changes, pkey changes, drops, null/not null changes), but more importantly, the data migration framework is meant to be outside, and manual... leading to problems, inconstancies, and more importantly, difficulties.
>
> The "rename" the old entity, create a migration service, then manually run it, is a "weak" migration framework.
>
> I have had success when trying to keep a tighter lid on things:
>
> 1- automatically create .sql migration files representing the differences in schemas. (rev<n> - rev<n-1> differences).
> 2- automatically add insert/update/delete commands representing the differences in seed into these files.
> 3- manually edit these files, or generate new ones to represent migration of data, where the "simple" cases are not handled, such as moving data from one entity, to another, etc...
> 4- these sql migration commands become formally part of the codebase.  Migration scripts automatically run on older schema versions (trick is the detect what version the schema actually is.... can't rely on version number, etc... too easy to be inaccurate, and run all "needed" migration commands).
> 5- Only supporting the "current" schema in the code base.  This means that as the schema evolves, code that isn't in the project will need to be upgraded when merging in these changes.
>
> Obviously the above series of steps are meant to move all "legacy" databases through an upgrade process to the current revision, such that they should be indistinguishable from being created from a newly minted schema.
>
> I have done this type of support in my past life, with another project, other than ofbiz and it helped tame the affects of schema changes.  We also have developed this approach at Emforium to manage the upgrade of our customer's ofbiz instances.  It has been VERY DIFFICULT to keep these instances consistent with one another, as the schema has changed over time.... I won't say that this is 100% nailed down, but we are on our way.
>
> So, I'd be interested in discussing how we can add a more formal migration declaration into ofbiz, with forward and backward migrations formally put into place (like ruby on rails, etc...).
>
> We can talk about using our developed framework for this, but there are limitations; such as .sql files are not likely portable, and certainly can't be easily split up between datasources.
>
> We've also generated a number of self tests, to test that a db at rev<n-1> can be upgraded to rev<n> with the seed to be identical, this is CRUCIAL to ensure, that the migration chain is unbroken...

I've got code started to handle this, from a few months ago.

<UpgradeScript id="FooBarBaz" location="component://$name/path/file.ext"/>

Then, at startup, a separate, special delegator is created very early,
which uses a private datastore(derby, postgres, or whatever).  Any
newly imported UpgradeScripts are then run just this once.

My code had support for script dependencies, ala debian, and could
order things appropriately.  The idea is based on another package I
saw developed, which did database stuff the old fashion way, with a
bunch of sql scripts individually developed for each database vendor.
 The package stored the current version somewhere, and it just ran all
upgrade scripts in series from the last installed version to the new
version.  This would handle multi-year upgrades, but it required
keeping those upgrade scripts around forever.  However, the scripts
were isolated, so the mainline code was kept clean.

[Tim Ruppert]

On Mar 18, 2010, at 7:57 PM, Adam Heath wrote:

> Marc Morin wrote:
>> I loath to enter into this discussion, but feel that I need to put in a bit of experience that I have observed when dealing with databases and migration issues.
>>
>> There are a couple of truisms that I have come to accept, grudgingly:
>>
>> 1) Database schemas will change over time.
>> 2) Database schema changes are a pain to deal with.  Migration issue are difficult.
>> 3) Unlike changes in code, that have no "history" to them (other than an interface specification to external api), database changes, have a long time horizon.  Decisions that were rational that lead to a schema design at the time, but new use cases, etc.. result in re-factoring or enhancing cause you to continually deal with the implications of this "old" design.
>> 4) It's impossible to satisfy all schema API revision constraints at the same time.
>> 5) You need to have a formal way to manage migration of data in your framework.
>> 6) Only support the newest schema, don't leave legacy artifacts around in schema, tends to increase duplication, increased confusion, and increased "code entropy".
>> 7) Avoid schema as an API point in your system if at all possible... can really tie your hands from a development point of view.  Means other systems will need to be updated when merging...
>>
>> So, Ofbiz doesn't really have any formal migration process. checkdb() adds missing stuff, but is incomplete in some changes (column type changes, index changes, pkey changes, drops, null/not null changes), but more importantly, the data migration framework is meant to be outside, and manual... leading to problems, inconstancies, and more importantly, difficulties.
>>
>> The "rename" the old entity, create a migration service, then manually run it, is a "weak" migration framework.
>>
>> I have had success when trying to keep a tighter lid on things:
>>
>> 1- automatically create .sql migration files representing the differences in schemas. (rev<n> - rev<n-1> differences).
>> 2- automatically add insert/update/delete commands representing the differences in seed into these files.
>> 3- manually edit these files, or generate new ones to represent migration of data, where the "simple" cases are not handled, such as moving data from one entity, to another, etc...
>> 4- these sql migration commands become formally part of the codebase.  Migration scripts automatically run on older schema versions (trick is the detect what version the schema actually is.... can't rely on version number, etc... too easy to be inaccurate, and run all "needed" migration commands).
>> 5- Only supporting the "current" schema in the code base.  This means that as the schema evolves, code that isn't in the project will need to be upgraded when merging in these changes.
>>
>> Obviously the above series of steps are meant to move all "legacy" databases through an upgrade process to the current revision, such that they should be indistinguishable from being created from a newly minted schema.
>>
>> I have done this type of support in my past life, with another project, other than ofbiz and it helped tame the affects of schema changes.  We also have developed this approach at Emforium to manage the upgrade of our customer's ofbiz instances.  It has been VERY DIFFICULT to keep these instances consistent with one another, as the schema has changed over time.... I won't say that this is 100% nailed down, but we are on our way.
>>
>> So, I'd be interested in discussing how we can add a more formal migration declaration into ofbiz, with forward and backward migrations formally put into place (like ruby on rails, etc...).
>>
>> We can talk about using our developed framework for this, but there are limitations; such as .sql files are not likely portable, and certainly can't be easily split up between datasources.
>>
>> We've also generated a number of self tests, to test that a db at rev<n-1> can be upgraded to rev<n> with the seed to be identical, this is CRUCIAL to ensure, that the migration chain is unbroken...
>
>
> I've got code started to handle this, from a few months ago.
>
> <UpgradeScript id="FooBarBaz" location="component://$name/path/file.ext"/>
>
> Then, at startup, a separate, special delegator is created very early,
> which uses a private datastore(derby, postgres, or whatever).  Any
> newly imported UpgradeScripts are then run just this once.
>
> My code had support for script dependencies, ala debian, and could
> order things appropriately.  The idea is based on another package I
> saw developed, which did database stuff the old fashion way, with a
> bunch of sql scripts individually developed for each database vendor.
> The package stored the current version somewhere, and it just ran all
> upgrade scripts in series from the last installed version to the new
> version.  This would handle multi-year upgrades, but it required
> keeping those upgrade scripts around forever.  However, the scripts
> were isolated, so the mainline code was kept clean.
>

This sounds really promising Adam.

[Jacopo Cappellato-4]

On Mar 18, 2010, at 9:57 PM, Adam Heath wrote:

> Jacopo Cappellato wrote:
>> Hi Ean,
>>
>> On Mar 18, 2010, at 7:05 PM, Ean Schuessler wrote:
>>
>>> Jacopo Cappellato wrote:
>>>> What I find interesting is that your OrderShipment entity was not empty.
>>>>
>>> Why is it unusual for data to be in that table? Sales orders on trunk are generating data in that table. Numerous services and screens use the entity.
>>
>> OrderShipment is used now, because I refactored the code in that commit; it was not used before that (apart from the "shipment plan" screen I mentioned, but as I said I doubt companies could use it in production).
>
> That's not correct, it was used at least by Sept 13, which is the
> version of ofbiz that my importer was originally run on, and that I
> used to figure out what I had to do.  I used ecommerce to see how
> orders were created.
>

ecommerce orders didn't create records in the OrderShipment entity; the saame is tru for the shipment's "Order Items" subscreen or the "quick ship order" screen.
The only way, that I am aware of, for storing data in that entity was to go to the shipment's "shipment plan" screen. I guess you did something like this

Jacopo

[Jacopo Cappellato-4]

On Mar 18, 2010, at 10:01 PM, Adam Heath wrote:

> Jacopo Cappellato wrote:
>> On Mar 18, 2010, at 6:37 PM, Adam Heath wrote:
>>
>>>> It may be that your change isn't really needed.  But I will admit I
>>>> don't have a complete knowledge of everything.
>>> OrderItemShipGroupAssoc?
>>
>> Is it a question for me?
>
> Maybe that's the entity that is actually used, meaning you wouldn't
> need to do that change you did.
>

This is very basic order data model information:
OrderItemShipGroupAssoc is used to group together order items (OrderItem) into ship groups (OrderItemShipGroup): each ship groups share the same set of preferences (ship to address, shipment method, estimated delivery date etc...); they are created before the shipment. They represent the agreed upon shipment information.
The same OrderItem could be associated to two (or more) different shipment groups, and this is the reason I have added the field to the pk of OrderShipment.
OrderShipment is used, when a shipment is created to associate a shipment item to the (part of the) order item assigned to that shipment.
Before my change, this entity was not used (apart from the "shipment plan" workflow) because the association was created and stored when ItemIssuance records were created i.e. when the items were issued from the warehouse and assigned to the shipment.
But if you wanted to create shipment items before the items were issued you had to create OrderShipment records.
Before my change, if an order item is split into two ship groups, OrderShipment was not able to tell you for what ship group the shipment items were for.

Jacopo

[Adam Heath-2]

Jacopo Cappellato wrote:

> On Mar 18, 2010, at 9:57 PM, Adam Heath wrote:
>
>> Jacopo Cappellato wrote:
>>> Hi Ean,
>>>
>>> On Mar 18, 2010, at 7:05 PM, Ean Schuessler wrote:
>>>
>>>> Jacopo Cappellato wrote:
>>>>> What I find interesting is that your OrderShipment entity was not empty.
>>>>>
>>>> Why is it unusual for data to be in that table? Sales orders on trunk are generating data in that table. Numerous services and screens use the entity.
>>> OrderShipment is used now, because I refactored the code in that commit; it was not used before that (apart from the "shipment plan" screen I mentioned, but as I said I doubt companies could use it in production).
>> That's not correct, it was used at least by Sept 13, which is the
>> version of ofbiz that my importer was originally run on, and that I
>> used to figure out what I had to do.  I used ecommerce to see how
>> orders were created.
>>
>
> ecommerce orders didn't create records in the OrderShipment entity; the saame is tru for the shipment's "Order Items" subscreen or the "quick ship order" screen.
> The only way, that I am aware of, for storing data in that entity was to go to the shipment's "shipment plan" screen. I guess you did something like this

You've piqued my interest.  I'm going to check out the previous
version of ofbiz that I used, and try it out again.

If I'm right, then it means that we may not need this change, and so
wouldn't have to even bother doing a deprecation, which is a much
simpler thing to do.

[Ean Schuessler]

Jacopo Cappellato wrote:
> OrderShipment is used now, because I refactored the code in that commit; it was not used before that (apart from the "shipment plan" screen I mentioned, but as I said I doubt companies could use it in production).
>  
I understand. I thought you were asking why his importer would use that
entity.

You were making the point that the normal deprecation procedure was
unnecessary because there wasn't any code in trunk that populates this
entity. I mostly agree with you but this entity has been in place for
years and is documented in the Data Model Resource Book. If the entity
was a recent OFBiz invention (ie. post 09.04) and had never been used by
code in trunk then I would agree with you 100%.

The deprecation procedure as currently specified leaves much to be
desired but its all we currently have and it is the policy.

--
Ean Schuessler, CTO
[hidden email]
214-720-0700 x 315
Brainfood, Inc.
http://www.brainfood.com

[Jacopo Cappellato-4]

On Mar 19, 2010, at 6:11 PM, Ean Schuessler wrote:

> Jacopo Cappellato wrote:
>> OrderShipment is used now, because I refactored the code in that commit; it was not used before that (apart from the "shipment plan" screen I mentioned, but as I said I doubt companies could use it in production).
>>  
> I understand. I thought you were asking why his importer would use that entity.
>
> You were making the point that the normal deprecation procedure was unnecessary because there wasn't any code in trunk that populates this entity.

This is not exactly what I wrote:

"no code was using the OrderShipment entity, no code apart from some old code (that I wrote) related to manufacturing shipment plans [...]"

> I mostly agree with you but this entity has been in place for years and is documented in the Data Model Resource Book.

Yes, I contributed this work a lot of time ago indeed.

> If the entity was a recent OFBiz invention (ie. post 09.04) and had never been used by code in trunk then I would agree with you 100%.
>
> The deprecation procedure as currently specified leaves much to be desired but its all we currently have and it is the policy.

Agreed. Btw, (and this question is not specifically addressed at you, Ean) when did it happen that our best practices became "policies"? Moreover, I don't remember of a vote to establish, not only this, but any of the "policies" and "rules" so frequently mentioned in this list,

Kind regards,

Jacopo

>
> --
> Ean Schuessler, CTO
> [hidden email]
> 214-720-0700 x 315
> Brainfood, Inc.
> http://www.brainfood.com
>