ledgersmb / LedgerSMB

Double-entry accounting & ERP for the web
https://ledgersmb.org
Other
415 stars 150 forks source link

Custom extensions block upgrades #2037

Closed einhverfr closed 8 years ago

einhverfr commented 8 years ago

Via email

OK, now I hit another problem.

setup.pl gave this error,

psql:sql/modules/Duplicates_Functions.sql:33: ERROR: Duplicate functions found: (public,gpr_consistent,boolean,"internal, prefix_range, smallint, oid") at /home/jasonic/perl5/perlbrew/perls/perl-5.20.0/lib/site_perl/5.20.0/PGObject/Util/DBAdmin.pm line 265.

which turns out to be because Duplicates_Functions.sql doesn't know about multimethods. I have a couple, related to some other stuff in the database, and it returned as a happy camper.

Schema |      Name      | Result data type |               Argument data types              
--------+----------------+------------------+-------------------------------------------------
 public | gpr_consistent | boolean          | internal, prefix_range, smallint, oid
 public | gpr_consistent | boolean          | internal, prefix_range, smallint, oid, internal
 public | prefix_range   | prefix_range     | text
 public | prefix_range   | prefix_range     | text, text, text

so I disabled that function in the LOADORDERS file.

This is going to be a viciously difficult problem to solve in its current form and banning all extra extensions is likely to cause more problems than it is worth.

Ideas:

  1. Blacklist instead of whitelist
  2. Fully persist the whitelist and give administrators the option of whitelisting the functions.

I do think this is blocking though

einhverfr commented 8 years ago

My proposed solution and I am happy to take it on after the printing 0 amounts on checks issue....

  1. Move from a whitelist to a blacklist
  2. Add a make target to generate the blacklist
  3. Add a test to check that that blacklist is up to date

This would allow this to remain as a hard failure would affect only to our own functions, and would ensure that those running from vcs are fully protected. Thoughts?

ylavoie commented 8 years ago

Both have pros & cons and would need maintenance but after a bit of thinking, blacklisting is probably better.

If we keep the whitelist, then I'd agree with your previous suggestion to make it a permanent table and I would add a mecanism for the administrator to white list his. We already have something similar to make sure that invoices are uniquely numbered when migrating. But then, we have the risk that someone uses it to white list a forgotten one of ours and that it fails downstream much later. This was what we were trying tp prevent.

Going to a black list seems easier and a suitable mean will have to be implement to find our functions and enlist them. That should not be a hard task though.

System.sql had some Perl functions whitelisted, we will have to revise the original test.

Your suggestions and help are always welcomed.

einhverfr commented 8 years ago

Thanks! I will probably work on this tomorrow. Will try to make sure that test failures are informative to new developers also.

einhverfr commented 8 years ago

Ok so a few pieces to this

  1. create the blacklist
  2. add makefile target
  3. add test with good error message when out of date.
  4. change system.sql and duplicate functions to use blacklist

First part is now done in my branch.

einhverfr commented 8 years ago

makefile now added. Next for tests

einhverfr commented 8 years ago

Plan for test is to take the sha1 of the file then re-run the blacklist building then compare the sha1 again if they are different we fail.

I am not sure how this works with OSX.....

einhverfr commented 8 years ago

ok so planning instead in both cases to chomp lines and concat.