Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Store values for callbacks on insert/update/delete operations #431

Merged
merged 3 commits into from
Dec 20, 2016

Conversation

gi0baro
Copy link
Member

@gi0baro gi0baro commented Dec 2, 2016

Ok everybody, this replace the current behaviour of Table._listify:

  • all the values computed before callbacks are passed to callbacks
  • removed duplicated logic for default fields: removed _defaults method
  • all the fields that have callable values are computed in the same flow (defaults, updates, computations)

Rationale:

Breaking change:

  • If you accessed a callable default or update value in callbacks you had to invoke that within the callback to get the real value. Now is just the real value.

Even if this is a breaking change, I consider the old behaviour wrong, since, for example, if you had a created_at field with default=lambda: datetime.utcnow(), and in an after_insert callback you wanted to access that value in the fields, you simply couldn't, since the real value inserted calling the method is not stored in the flow.

I would like to hear @mdipierro, @niphlod, @leonelcamara on this.

This changed also the flow regarding Table._attempt_upload, since now is a callback method in the before_insert and before_update. So while the old flow was:
_attempt_upload, callable_values, before_callback, operation

the new one is:
callable_values, before_callback, operation

@codecov-io
Copy link

codecov-io commented Dec 2, 2016

Current coverage is 68.68% (diff: 66.49%)

Merging #431 into master will increase coverage by 0.18%

@@             master       #431   diff @@
==========================================
  Files            69         69          
  Lines          9102       9163    +61   
  Methods           0          0          
  Messages          0          0          
  Branches       1987       1987          
==========================================
+ Hits           6235       6294    +59   
- Misses         2345       2347     +2   
  Partials        522        522          

Powered by Codecov. Last update 5cd99f5...04c35cd

@gi0baro
Copy link
Member Author

gi0baro commented Dec 6, 2016

@mdipierro @niphlod @leonelcamara ..comments?

@niphlod
Copy link
Member

niphlod commented Dec 6, 2016

bummer, I reeeeaally don't have time to scour the code. IMHO both the rationale and the breaking change is fine.
I don't really have the time to assert where _attempt_upload should be.
Breaking something that theoretically should be atomic (the CRUD operation in the db AND the CRUD operation in the file) in all code-paths (stored to disk, stored in a virtual FS, converted to base64 and injected in another column) is something that, no matter how, is really error-prone given the flexibility uploads have (don't even get me started on storing the upload in a real binary field, which is a huuuuuge miss on dal/pydal since its conception).
The only thing I'm midly sure about is that NOT needs to happen after the _before_callback (or, in reverse words, it needs to happen either before or in _before_callback)

@gi0baro
Copy link
Member Author

gi0baro commented Dec 10, 2016

@niphlod I've just moved the attempt_upload into the before callbacks.

@mdipierro @leonelcamara I consider this ready to be merged. Still waiting for your comments.

@gi0baro gi0baro changed the title [WIP] Store callable values on insert/update/delete operations [WIP] Store values for callbacks on insert/update/delete operations Dec 10, 2016
@gi0baro gi0baro changed the title [WIP] Store values for callbacks on insert/update/delete operations Store values for callbacks on insert/update/delete operations Dec 10, 2016
@gi0baro gi0baro added this to the 16.12 milestone Dec 10, 2016
@gi0baro
Copy link
Member Author

gi0baro commented Dec 20, 2016

@mdipierro I will merge this by Thu 22/12 unless you or @leonelcamara have any comments.

@mdipierro
Copy link
Contributor

LGTM

@mdipierro mdipierro merged commit e49dd9b into web2py:master Dec 20, 2016
valq7711 added a commit to valq7711/pydal that referenced this pull request Aug 26, 2017
* added codecov settings file

this should silence unnecessary messages from codecov. we can then
adjust settings going forward

* checkpoint

* possibly fixed the issue with id in import_from_csv

* fixed import_from_csv_file

* removed unwanted print

* another import fix

* more import debugging

* Enabled entity quoting as default also on DAL class (web2py#365)

* Enhancement/teradata lastrowid (web2py#363)

* Initial change to remove ESCAPE from teradata syntax

* Revert "Initial change to remove ESCAPE from teradata syntax"

This reverts commit 81489e9.

* Fix lastrowid error for Teradata adapter

* further update to teradata adapter

* Allow custom Rows classes (web2py#367)

* Added Rows.append and Rows.insert (web2py#366)

* [WIP] Indexes support (web2py#361)

* First implementation of web2py#152

* Returning True on index creation/drop

* Added where option on postgre create_index

* Added tests for indexes

* Added support for additional arguments to Table.create_index

* Better expansion in sql instructions for indexes, better errors for indexes

* Re-implemented _lastsql on DAL class (web2py#368)

* Fixed web2py#319 and web2py#333

* turn off codecov/changes report

hopefully :)

* Some fixes on StringIO usages with py3

* Better blobs handling with MongoDB and python3

* Added missing representation of 'password' fields on NoSQL adapters

* Updated mongoDB tests on py3 and blobs

* Updated changelog

* Releasing 16.06

* Added postgres3 adapters, put back multiple values type boolean parsing in base parser

* Removed postgres2 adapter from tests matrix since implicitly tested on postgres3

* Updated dev version

* Making connection and cursors fork-safe in multiprocess environments

* Fixing missing inheritance of additional fields expressions in dialects

* Releasing 16.06.09

* Updated dev version

* Added forced parsing of date and datetime fields in SQLite parser

* Added aggregation type detection test for fields

* Fix web2py#376

* Fixed web2py#377

* default quote_template for CommonDialect

* Enhance d13e82f

* Fix web2py#380

* Releasing 16.06.20

* Updated dev version

* fixes web2py#382

* Improving parsing performance (web2py#385)

* Fix web2py#383

* Releasing 16.06.28

* Updated dev version

* added rows.join('name',lambda ids: db.referenced.field.belongs(ids))

* remove test that depends on details of json serialization

* fixed removal order of new table in test

* yet simpler and smarter row.join

* fixed rows.join(...)

* one more improvement in join

* allow many2many in row.join

* ignore pool_size for sqlite

* better security

* enabled python3.5 on travis (web2py#393)

* fix py35 Field.__hash__(), close web2py#392 (web2py#394)

* added py35 to tox.ini

* Avoid to run travis tests on py35 and google

* fixes web2py#396

* Skip travis tests on python 3.3

* Releasing 16.07

* Updated dev version

* Fix web2py#399 - postgres adapter lastrowid() to use currval(seq) instead of lastval()

* reinstated self._last_insert, no need to remove working logic

* defied long in _compat

* merged pydal

* portalocker: removed pywin32 dep, added tests (web2py#401)

* Avoid upload errors on py3 if upload content is a bytes string

* Compat module cleaning

* Releasing 16.08

* Updated dev version

* MongoDB: testcase and fix for query(field==list:reference)

* Enhancement/teradata test (web2py#413)

* Initial change to remove ESCAPE from teradata syntax

* Revert "Initial change to remove ESCAPE from teradata syntax"

This reverts commit 81489e9.

* Initial teradata tests changes.  More to come

* first sql.py change, TestAddMethod now OK

* include teradata in setup

* Postgres fixes (web2py#414)

* Fixed postgres2 and postgres3 adapters inheritance, updated `PostgreMeta` to choose the right class depending on uri

* Fixed representation for lists on postgres2 and postgres3 adapters

* Fixed typo in PostgreDialectArrays

* Ensure strings in PostgreArrayRepresenter._listify_elements

* New test helper class DALtest that handles connection boilerplate (web2py#415)

* Enhancement/teradata test (web2py#418)

* Initial change to remove ESCAPE from teradata syntax

* Revert "Initial change to remove ESCAPE from teradata syntax"

This reverts commit 81489e9.

* TestBelongs for Teradata

* More Teradata test changes

* run the test.

* fix for opening files in py3 (web2py#420)

* fix for opening files in py3

fixes also web2py/web2py#1451

* fix for PY2 detection

* Releasing 16.09

* Updated dev version

* Enhancement/teradatatest (web2py#426)

* Initial change to remove ESCAPE from teradata syntax

* Revert "Initial change to remove ESCAPE from teradata syntax"

This reverts commit 81489e9.

* Skipping TestChainedJoinUnique for Teradata.

* Clean connection data on thread local on DAL.close (web2py#427)

* Releasing 16.11

* Updated dev version

* Joinable subselects (web2py#408)

* Minimal implementation of joinable subselects (work in progress)
* Generate subselects in Set.nested_select() instead of BaseAdapter._select()
* Add support for subselects to BaseAdapter._count()
* Prevent table name collisions when applying record versioning filters
* Fix common filters in select with subselects
* Add support for subselect fields to Rows.render() and fix bugs
* Refactor Google datastore adapter and BaseAdapter.get_table()
* Check for table name collisions when building a SELECT query
* When building subselect string, ignore cache if outer_scoped is not empty
* Minor optimization in SQLAdapter._select_wcols()
* Add parameter "correlated" to Select constructor defaults to True. When set to False, the subquery will be self-contained, meaning it will never reference any tables from the parent scope.
* Implement proper expansion of correlated subqueries
* Check that subquery in belongs() expression has exactly 1 column
* Implement expansion of correlated subqueries in count(), update(), delete()
* Update Mongo unit tests to new adapter API
* Move part of Set.nested_select() to adapter code
* Unit tests for the Select class
* Additional unit tests for fixed bugs
* Expand aliased table names using dialect methods
* Fix expansion of correlated subqueries used in UPDATE/DELETE statements

* Refactor table name attributes and fix related aliasing/rname bugs

* Update IMAP adapter methods to new API

* Add table alias support to UPDATE/DELETE in MSSQL dialect

* Store values for callbacks on insert/update/delete operations (web2py#431)

* Changed listify to store computations in values

* Moved attempt_upload logic to callbacks

* Fixed callbacks access to fields

* One more check that Table.with_alias() updates all field attributes

* Rename Table.sqlsafe and Table.sqlsafe_alias

* Refactor field name attributes and fix related rname bugs

* Fix type error in MSSQL TFK reference template

* Use the correct constraint name in MSSQL TFK references

* Added missing 'OpRow.get' method

* Fixed after_update callbacks argument in validate_and_update

* tablemap should contains all tables to avoid key errors in adapter _select_wcols

* Fix field migrations and some final refactoring

* Fixed wrong add operation with float types web2py#445

* Updated changelog

* Releasing 17.01

* Fix DAL(None).define_table()

* Fix perpetual migrations due to changes in name quoting

* Fix storing Rows objects in pickle-based cache

* Pass the list of field references when creating a subset of Rows object

* Disable part of pickling test on MSSQL, driver problem...

* Completely disable the pickling test on MSSQL (driver problem...)

* Updated dev version

* Correctly configure adapters that need connection for configuration

* Added locking for reconnect method switch, created a mixin for adapters that need reconnection mocking

* added tests for py36

* Always find the driver on adapters

* Cleaner solution for web2py#446

* Updated changelog

* Releasing 17.03

* added missing "oracle" in adapters.register_for() (web2py#457)

Fixes web2py/web2py#1599

* field.set_attributes(..) could return the instance (web2py#458)

Would let the code be  more straightforward -- example, when passing the "decorated" field to the function...
Now I have to   "scatter" the code in two places for each field...

* testing py36 on appveyor (web2py#456)

* Fix get in pydal.helpers.classes Reference class calling __getattr__ … (web2py#465)

* Fix get in pydal.helpers.classes Reference class calling __getattr__ with 3 variables instead of 2 as per signature

* Move fix of wrong method call in Reference class from get to __getattr__

* Make DAL support jsonb for postgres (web2py#469)

* Make DAL support jsonb for postgres

* added parser for jsonb

* experimantal code to address issue #1613

* fixed error introduced in recent refactoring by me

* mysql and postgres unix_socket support

* fixed _find_work_folder

* support for gcloud sql v2. thanks Karoly Kantor

* small typo in warnings

* Releasing 17.07

* fixed issue web2py#480

* Fixes web2py/web2py#1683 (web2py#484)

* allow DAL(..., adapter_args=dict(migrator=InDBMigrator))

* fixed custom migrator

* Updated dev version

* Cleaning up f71aade

* Fix Table objects in common fields (web2py#486)

This fixes the problem of putting and tables on common_fields and having lazy_define_table throw an exception because Table doesn't have a clone attribute.

* fixed typo changing `oracle_fix` to `cmd_fix` in adapters/oracle.py (web2py#471)

fixed error where %-formats were only applying to second part of concatenated SQL statement in dialects/oracle.py

* override _select_aux_execute method in adapters/oracle.py to use oracle-specific `fetchall` (web2py#473)

* fixed MSSQL TOP

* possible fix for tTIZLy33Yw8

* fixed del of missing key, thanks Lazaro Aulan

* Fixed st_asgeojson (web2py#489)

see https://groups.google.com/forum/#!topic/web2py/EixMPrPr9SY
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants