Forum OpenACS Development: upgrade-5.9.1d15-5.9.1d16.sql breaks acs_rel_type__create_type

Hi!

11 ]project-open[ packages don't install anymore after running:
/acs-kernel/sql/postgresql/upgrade/upgrade-5.9.1d15-5.9.1d16.sql
from OpenACS 5.9.1.

This upgrade script creates a new variant of acs_rel_type__create_type which have a default value "composable_p boolean default true" as their last argument.

This leads to the situation that the database sees an ambiguity with the existing ]po[ calls.

Error message:
psql:/web/projop/packages/intranet-budget/sql/postgresql/intranet-budget-create.sql:485: ERROR: function acs_rel_type__create_type(unknown, unknown, unknown, unknown, unknown, unknown, unknown, unknown, unknown, integer, unknown, unknown, unknown, integer, unknown) is not unique
LINE 1: select acs_rel_type__create_type (
^
HINT: Could not choose a best candidate function. You might need to add explicit type casts.

This ambiguity leads to the situation that the entire ]po[ installation has stopped working. We would have to change 11 packages to adapt to the new situation.

Would it be possible to roll-back these incompatible changes and/or correct them in the next OpenACS version? I understand that breaking compatibility was not the intention of the code change, correct?

Cheers
Frank

I just tried a workaround adding a ",true" to these calls in the 11 ]po[ packages, but doing so breaks installability on ]po[ V5.0 or earlier that use OpenACS 5.6.0.
Dear Frank,

is it possible that ]po[ ships a custom 15 arguments version of acs_rel_type__create_type? This seems to me the simplest explanation for your issue.

Please double check the definitions of this function in your code or in the database and make sure this was not extended/customized downstream, we can give more suggestions from there.

Ciao

Antonio

Hii Antonio,

Thanks for the quick response and your time!

15 arguments version

That is correct. But it was not customized.

We use exactly the same acs_rel_type__create_type since ACS 3.4 (before OpenACS!). Here is an example:

select acs_rel_type__create_type (
'im_agile_task_rel', -- relationship (object) name
'Agile Task Rel', -- pretty name
'Agile Task Rel', -- pretty plural
'relationship', -- supertype
'im_agile_task_rels', -- table_name
'rel_id', -- id_column
'intranet-agile', -- package_name
'im_project', -- object_type_one
'member', -- role_one
0, -- min_n_rels_one
null, -- max_n_rels_one
'acs_object', -- object_type_two
'member', -- role_two
0, -- min_n_rels_two
null -- max_n_rels_two
);

You must have had a very good reason to break this compatibility? Maybe you can explain the reasoning behind? And was it really necessary to delete the previous version of the call? Why not create a different signature of the procedure to maintain compatibilitiy?

This is the 3rd or 4th incompatible change that I've seen in the last years. I'm not sure about the reasoning process behind this. We actually did not upgrade to OpenACS 5.10 because there were additional errors due to incompatibilities.

My point is: Please check with "us users" of OpenACS before performing incompatible changes. I know that Quest has similar issues, and I understand there are several other projects depending on a stable API.

I can offer you a "test lab in a box" (or better "in a VM") to allow you to quickly check if a new OpenACS version is compatible or not.

Cheers
Frank

Frank, why you are getting so quickly personal? It is always better to stay on the constructive side.

The changes you are talking about a happened in 2016 (many years ago), and the main changes was committed by me [1] and was based on a contribution that was discussed before in the forums, and was as well announced after the change in the forums [2].

The file where PO is having problems (intranet-budget-create.sql) does not seem to be in a public repository (at least i could not find it), so this seems to be one of the for-money packages of PO. Anyhow, from your code snippets, i think, that the commit [1] with the dotlrn changes shows you the way: in cases, where max_n_rels is null, add an integer cast to it, such that the function resolution via types works [3]. This will make PO work with composable rel-types, and it is be fully backward compatible.

Technically, the best approach would be to switch to named attributes, where PostgreSQL 9.5 introduced a syntax compatible with Oracle, where the number of variants could be significantly reduced, and the insecure function resolution based on types (which are often not provided) could be avoided. But i do no know anybody, who would invest in this direction.

all the best
-gn

[1] https://github.com/openacs/dotlrn/commit/84c5854d48b8a7ff6a37e689243062c4815a959e
[2] https://openacs.org/forums/message-view?message_id=5330734
[3] https://github.com/openacs/dotlrn/blob/oacs-5-9/sql/postgresql/class-memberships-create.sql#L128

Hi Gustaf!

Happy Holidays!
Sorry if the last post felt "personal", it was intended to be factual.

My point is: An API (and the PL/SQL functions are kind of an API) is supposed to be stable, and breaking it is sometimes necessary, but there should be a good reason.

My personal impression is that there are "too many" broken API calls without good reason in the last 2-3 years. That's personal (the impression)! :-) Maybe I just don't understand the reasoning behind it, because I'm not enough involved in the "upstream" OpenACS development and the use-cases you are dealing with. I just know how careful we are not to break compatibility, maybe I'm using the same expectations towards OpenACS, and that is incorrect for some reason.

We've got 9 broken cases of acs_rel_type__create_type plus some update scripts:
./intranet-sla-management/sql/postgresql/intranet-sla-management-create.sql
./intranet-release-mgmt/sql/postgresql/intranet-release-mgmt-create.sql
./intranet-helpdesk/sql/postgresql/intranet-helpdesk-create.sql
./intranet-budget/sql/postgresql/intranet-budget-create.sql
./simple-survey/sql/postgresql/simple-survey-create.sql
./intranet-confdb/sql/postgresql/intranet-confdb-create.sql
./intranet-mail-import/sql/postgresql/intranet-mail-import-create.sql
./intranet-agile/sql/postgresql/intranet-agile-create.sql
./intranet-core/sql/postgresql/intranet-biz-objects.sql

We can fix this in a few hours, it's not such a big problem. However:
- I believe it's import to report such cases, isn't it? Is there a better way to do this?
- My impression is that the frequency of broken API issues is increasing. I'm not sure about this, it's just an impression.

My point is: Please check with "us users" of OpenACS before performing incompatible changes.
I know that Quest has similar issues, and I understand there are several other projects depending
on a stable API.
I can offer you a "test lab in a box" (or better "in a VM") to allow you to quickly check if a new
OpenACS version is compatible or not.

Can we find a way to test new OpenACS versions for compatibility without much effort for your team or us?
I offer to setup a separate VM only for compatibility testing. You would notify us when there is a new upcoming version, we'd check for compatibility and you would make sure things work again? We could do the work, or provide you with a the environment, or however you'd prefer...

Writing about bugs always is a bit negative, so I want to state that we are very, very happy that there is active OpenACS development from your side.

Happy new year!
Frank

Dear Frank,

we normally take care of not breaking compatibility with old code and we go some distance in order to achieve this: supporting legacy flags, providing fallback implementations for new api... and so on.

Of course there could be much more use cases in the wild that we sometimes assume, plus various mixtures of legacy/custom code and setups.

In this particular case, the change was quite subtle, as it would affect the type of the argument, rather than the argument itself, so the chances increase that some usage would break.

That people find and report issues is perfectly normal and healthy for a project. What is even healthier is contributing so that issues are fixed and/or detected earlier. Hence my suggestion, if you feel like offering some help, would be to write new automated tests, possibly involving the API you use the most or care the most about.

Running such tests, together with the ones already available, on a VM of yours on a regular basis (e.g. in a continuous integration pipeline) would also be much appreciated, as there are certainly features of your setup that might be different from, say, those on LEARN or a vanilla instance.

Happy new year to you too!

Ciao

Antonio

"too many" broken API calls without good reason in the last 2-3 years.
hmm, you are aware, that the discussion about the issue on hand goes back to 2003 (17years). The proposed fix was out since 2013 (7 years ago) and was committed finally in 2016. There was plenty of time to look at the changes and to consider consequences for other projects. ... Also be aware that this change was requested by users (as most of the changes).

At the time of the commit, the final solution was the best compromise between keeping high backward compatibility and functional extension. I did these changes for all of the ~100 maintained packages, the changes were very little and took me just a few minutes.

If you look at this change, it was necessary to introduce an additional parameter to the stored procedure (in PostgreSQL and Oracle). To keep the call highly compatible, the parameter was made optional. This works without maintenance cost for most of the cases, but due to the fact that there was before the change already versions of acs_rel_type__create_type with 15 and 16 arguments, with very similar argument types, it required casts in rare situations, where the types cannot be inferred from the values (in particular "null") for function disambiguation.

If one compares OpenACS with other frameworks (php, ruby, ... JavaScript) the stability of the interface is one of the highest around. Most compatibility problems are from sites, using deprecated interfaces, non-public interfaces, or local modifications. Often the sites are not aware of the deprecation. This improved over the years, since modern OpenACS provides log entries when deprecated API calls are used.

Part of the problem is as well that there is no API for "acs_rel_type__create_type" (a call, which does not exist as such on Oracle).... But even when we would introduce an API for this, using the API would require maintenance work. There are no calls deliberately altered. Backward compatibility is a high goal (but not the only one).

The OpenACS core team was always and is always open for collaboration. Collaboration requires effort from all sides, following what is happening in the community, reading commit messages, etc.

We are very, very happy that there is active OpenACS development from your side
Sometimes it is not so easy to read between the lines.

All the best in 2021!
-g