threatify / arango-orm

A SQLAlchemy like ORM implementation using python-arango as the backend library
GNU General Public License v3.0
148 stars 22 forks source link

Incorrect edge schema generation in 0.7.0 #114

Open volodymyrko opened 2 years ago

volodymyrko commented 2 years ago

Hi there seems be an issue with generating schema (CollectionBase.schema) for Edges/Relations with custom attributes (like SpecializesIn ) in specific case. Here are few examples:

  1. when schema method is run for SpecializesIn first (before any other relations) everything is fine
    
    (Pdb) self._graph.edges['specializes_in'].schema().fields.keys()
    <class 'tests.data.SpecializesIn'>
    dict_keys(['expertise_level', '_key'])   # 'expertise_level' is present

(Pdb) self._graph.edges['studies'].schema().fields.keys() <class 'arango_orm.collections.Relation'> dict_keys([])

2. if `schema` method is run for Relation first it  does not generate correct schema

(Pdb) self._graph.edges['studies'].schema().fields.keys() <class 'arango_orm.collections.Relation'> dict_keys([])

(Pdb) self._graph.edges['specializes_in'].schema().fields.keys() <class 'tests.data.SpecializesIn'> dict_keys([]) # IS EMPTY


is happens because `Relation._cls_schema` is already generated/cached at the first step (`self._graph.edges['studies'].schema()`) and it is used when `self._graph.edges['specializes_in'].schema()` is run. `SpecializesIn` is inherited from `Relation` so it uses  `Relation._cls_schema` data
SoundsSerious commented 1 year ago

Hi @volodymyrko

I agree there are some issues with the schema caching. This was something I implemented as I found repeated calls to the schema generated actually created a pretty significant memory leak.

I dont know what the right answer is, a couple ideas have been floated:

  1. get rid of marshmallow which has been causing some problems like the memory leak
  2. provide an explicit method to cache schema #119
  3. improve the caching #115
overbost commented 1 year ago

@volodymyrko can you check the last commit #122 (it's merged into master now). This should fix your issue.

volodymyrko commented 1 year ago

@overbost , my tests passed successfully. thanks

PTank commented 1 year ago

Hi, can you update the pypi package to the last version to fix this issue ? Thanks

kashifpk commented 1 year ago

@PTank Done. @PTank @volodymyrko @overbost @SoundsSerious Please use https://github.com/kashifpk/arango-orm for the future as I no longer have push access to this repo. I'll be pushing updates to this new repo in the future.