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

many-to-many nullable columns with through #852

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

nikelborm
Copy link

such code:

...
class TickerCollection(ormar.Model):
  class Meta(BaseMeta):
    tablename = "ticker_collection"

  id: int = ormar.Integer(name="ticker_collection_id", nullable=False, primary_key=True, autoincrement=True)

  createdByUser: Optional[User] = ormar.ForeignKey(User, name="created_by_user_id", nullable=True, unique=False)
  isPublic: bool                = ormar.Boolean(         name="is_public",          nullable=False, default=False)
  name: str                     = ormar.Text(            name="name",               nullable=False)
  description: str              = ormar.Text(            name="description",        nullable=True)
  tickers: Optional[List[Ticker]] = ormar.ManyToMany(
    to=Ticker,
    through=TickerToTickerCollection,
    related_name="tickerCollections",
    through_relation_name="ticker_collection_id",
    through_reverse_relation_name="ticker_id",
    # is_through_relation_column_nullable=False,
    # is_through_reverse_relation_column_nullable=False,
  )
...

cause such migration:

...
op.create_table('ticker_to_ticker_collection',
    sa.Column('id', sa.Integer(), nullable=False),
    sa.Column('ticker_id', sa.Integer(), nullable=True),
    sa.Column('ticker_collection_id', sa.Integer(), nullable=True),
    sa.ForeignKeyConstraint(['ticker_collection_id'], ['ticker_collection.ticker_collection_id'], name='asd1', onupdate='CASCADE', ondelete='CASCADE'),
    sa.ForeignKeyConstraint(['ticker_id'], ['ticker.ticker_id'], name='asd2', onupdate='CASCADE', ondelete='CASCADE'),
    sa.PrimaryKeyConstraint('id')
    )
...

Here ticker_id and ticker_collection_id are nullable by default, and there is no built-in way to set them non nullable.
I tried to set sql_nullable and nullable in arguments of ormar.ManyToMany but looks like it affects only typing information.
So then I added these two parameters, that will allow to override nullability of both through columns.

@collerek collerek force-pushed the many-to-many-nullable-columns branch from 77fae53 to 4b7de5a Compare December 10, 2022 16:22
Comment on lines +127 to +128
is_through_relation_column_nullable = kwargs.pop("is_through_relation_column_nullable", None)
is_through_reverse_relation_column_nullable = kwargs.pop("is_through_reverse_relation_column_nullable", None)
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you rename it to shorter names: through_relation_nullable and through_reverse_relation_nullable

)
create_and_append_m2m_fk(
model=model_field.owner, model_field=model_field, field_name=child_name
model=model_field.owner, model_field=model_field, field_name=child_name, nullable=model_field.is_through_relation_column_nullable,
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you add missing tests for setting that column settings nullable/ not nullable and different expected behavior?

@collerek
Copy link
Owner

Sorry for the long review time!

Copy link

codecov bot commented May 26, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 100.00%. Comparing base (7f3149d) to head (ccbd159).

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff            @@
##            master      #852   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files          204       204           
  Lines        14845     14847    +2     
=========================================
+ Hits         14845     14847    +2     
Files Coverage Δ
ormar/fields/many_to_many.py 100.00% <100.00%> (ø)
ormar/models/helpers/sqlalchemy.py 100.00% <ø> (ø)

Copy link

codspeed-hq bot commented May 26, 2024

CodSpeed Performance Report

Merging #852 will degrade performances by 12.33%

Comparing nikelborm:many-to-many-nullable-columns (ccbd159) with master (7f3149d)

Summary

❌ 1 regressions
✅ 83 untouched benchmarks

⚠️ Please fix the performance issues or acknowledge them on CodSpeed.

Benchmarks breakdown

Benchmark master nikelborm:many-to-many-nullable-columns Change
test_deleting_individually[10] 8.6 ms 9.8 ms -12.33%

@nikelborm
Copy link
Author

nikelborm commented May 26, 2024

Hi @collerek
Since I wrote this PR, long time passed. At the time I wasn't much experienced in python\ormar and didn't have much time, so I wasn't able to properly write tests on this PR and fix your reviews, but now I hope I'm soon will be able to finish it

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants