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

Collapse
5: Re: upgrade-5.9.1d15-5.9.1d16.sql breaks acs_rel_type__create_type (response to 1)
Posted by Gustaf Neumann on 12/29/20 06:28 PM
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

Collapse
6: Re: upgrade-5.9.1d15-5.9.1d16.sql breaks acs_rel_type__create_type (response to 5)
Posted by Frank Bergmann on 12/30/20 02:12 PM
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

Collapse
7: Re: upgrade-5.9.1d15-5.9.1d16.sql breaks acs_rel_type__create_type (response to 6)
Posted by Antonio Pisano on 12/30/20 07:17 PM
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

Collapse
8: Re: upgrade-5.9.1d15-5.9.1d16.sql breaks acs_rel_type__create_type (response to 7)
Posted by Gustaf Neumann on 01/03/21 10:38 AM
"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