Skip to content

Conversation

mahenzon
Copy link
Member

When specifying custom model id field using model_id_field_name there's an 500 internal server error when getting list of entities.

This PR fixes it using the provided models_storage.get_model_id_field_name

@mahenzon mahenzon requested a review from CosmoV March 22, 2025 16:13
@mahenzon mahenzon force-pushed the fix/read-custom-model-id-field branch 4 times, most recently from c68393f to 0062d52 Compare March 22, 2025 17:14
Copy link

codecov bot commented Mar 22, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 91.63%. Comparing base (1556af3) to head (cc52512).

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main      #98      +/-   ##
==========================================
+ Coverage   91.44%   91.63%   +0.18%     
==========================================
  Files          44       44              
  Lines        2408     2414       +6     
  Branches      279      279              
==========================================
+ Hits         2202     2212      +10     
+ Misses        143      142       -1     
+ Partials       63       60       -3     
Flag Coverage Δ
unittests 91.63% <100.00%> (+0.18%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines Coverage Δ
fastapi_jsonapi/api/schemas.py 100.00% <100.00%> (ø)
fastapi_jsonapi/data_layers/base.py 93.84% <100.00%> (+0.19%) ⬆️
fastapi_jsonapi/data_layers/sqla/base_model.py 89.79% <100.00%> (ø)
fastapi_jsonapi/data_layers/sqla/orm.py 89.30% <ø> (ø)
fastapi_jsonapi/views/view_base.py 93.08% <100.00%> (+0.03%) ⬆️

... and 2 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

cls,
session: AsyncSession,
stmt: Select,
resource_type: str,
Copy link
Collaborator

@CosmoV CosmoV Mar 26, 2025

Choose a reason for hiding this comment

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

Лучше делать методы по возможности идемпотентными. Тут запрос меняется в зависимости от состояния models_storage

Предлагаю передавать имя "id" поля в отдельном Optional аргументе как-нибудь так

@classmethod
async def count(
    ...,
    id_field_name: Optional[str] = "id",
):
    ...

Подозреваю, что вызов count всегда будет производиться с использованием того же resource_type, с которым инстанцирован DataLayer. Проверь пожалуйста этот момент и если это так нужно будет прокинуть id_field_name в DataLayer на этапе инстанцирования приложения и пользоваться этим значением

Copy link
Member Author

Choose a reason for hiding this comment

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

ок, переделал:

  • вынес получение id_column_name в базовый data layer
  • при вызове count передаю имя колонки

Теперь после инициализации data layer знает имя айди поля.
Data Layer инициализируется на каждый view заново, а View инициализируется на каждый запрос. DL привязывается к текущему view и работает с текущей сущностью

Copy link
Collaborator

@CosmoV CosmoV Apr 7, 2025

Choose a reason for hiding this comment

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

Тут надо пойти ещё дальше и передавать id_field_name на уровне view https://github.com/mts-ai/FastAPI-JSONAPI/blob/fix/read-custom-model-id-field/fastapi_jsonapi/api/endpoint_builder.py#L216 для каждого эндпоинта.

На момент создания эндпоинтов ModelsStorage содержит инфоррмацию по всем ресурсам, соответственно нет потребности вызывать его в рантайме.

Copy link
Member Author

Choose a reason for hiding this comment

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

если мы из DataLayer вынесем в view, то внутри DL как обращаться?
Ты предлагаешь добавить при инициализации view, а мы это делаем на каждый запрос. и внутри view инициализируем DL - получается, что так, что так мы будем выставлять это значение

id_field_name=self.id_column_name,

нужна ли доработка в итоге?

client: AsyncClient,
age_rating_g: AgeRating,
):
url = app.url_path_for("get_age-rating_list")
Copy link
Collaborator

@CosmoV CosmoV Mar 26, 2025

Choose a reason for hiding this comment

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

В этом кейсе проверяем только случай, когда запрашиваем модель на верхнем уровне. Давай подойдем к решению бага комплексно, а именно нужны кейсы на

  • запрос модели на верхнем уровне (этот случай)
  • корректное поведение модели с кастомным id в качестве релейшеншипа/инклюда
  • запрос модели через атомики

Copy link
Member Author

Choose a reason for hiding this comment

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

добавил создание с релейшном

создал отдельный тест на атомики

@mahenzon mahenzon force-pushed the fix/read-custom-model-id-field branch from 3fdaf94 to c55ab22 Compare March 29, 2025 16:02
+ other fixes
+ tests
@mahenzon mahenzon force-pushed the fix/read-custom-model-id-field branch from 29b6615 to cc52512 Compare March 29, 2025 17:04
)

path: Union[str, list[str]]
router: Optional[APIRouter]
Copy link
Collaborator

Choose a reason for hiding this comment

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

Зачем?

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