use populate_by_name=True in BaseModel

[rawwar]

related to ##44306

When working on #44306, noticed that we are dumping the model data and recreating ConnectionBody. But, issue here is that we dump the model data, output data will have "schema_". when this is passed to the ConnectionBody, its considered as a new filed and is ignored. Resulting, schema_ to be None. this PR fixes it by updating data using model_validator

[rawwar]

We can either fix this using model_validator or only update ConnectionBody(**patch_body.model_dump()) to below:

data = patch_body.model_dump()
data['schema'] = data.pop('schema_')
ConnectionBody(**data)

which one looks better, @pierrejeambrun , @bugraoz93

[bugraoz93]

We can either fix this using model_validator or only update ConnectionBody(**patch_body.model_dump()) to below:

data = patch_body.model_dump()
data['schema'] = data.pop('schema_')
ConnectionBody(**data)

which one looks better, @pierrejeambrun , @bugraoz93

Thanks for bringing this up! Model Validator approach seems better in this context. I think the implementation of the endpoints shouldn't intervene datamodels in the ideal scenario.
For the problem in general, do you think if we tweak the validation/serialization alias for datamodels, for example, for connection_id, will the problem still persist?

[rawwar]

For the problem in general, do you think if we tweak the validation/serialization alias for datamodels, for example, for connection_id, will the problem still persist?

At least in the ConnectionBody case, yeah. we can def just use conn_id(which is the column in db) instead of connection_id. we can just update tests to make it work. Should I make this change? Unless we need "connection_id" to be in the model

[pierrejeambrun]

Can't we just dump using by_alias=True ?

[rawwar]

Can't we just dump using by_alias=True ?

That won't work because of connection_id having serialization alias as conn_id . We can rename it as i mentioned in previous comment

[pierrejeambrun]

Do you mind highlighting the code snipped that is causing problem ?

Can't we populate_by_name ? To allow population by both alias and field name ? so shema_ and schema both are valid.

[rawwar]

Can't we populate_by_name ? To allow population by both alias and field name ? so shema_ and schema both are valid.

Didn't notice populate_by_name . This will work. Shall I add it to the BaseModel's ConfigDict?

Do you mind highlighting the code snipped that is causing problem ?

airflow/airflow/api_fastapi/core_api/datamodels/connections.py

Line 86 in 823ffde

class ConnectionBody(BaseModel):

Here we have connection_id with serialization_alias set to conn_id. Hence, we can't use by_alias=True.

[pierrejeambrun]

Didn't notice populate_by_name . This will work. Shall I add it to the BaseModel's ConfigDict?

I don't know. I don't think we need that in the base model yet. (edited: if the CI is happy with it lets do that)

Do you mind highlighting the code snipped that is causing problem ?

I was talking about the usage of the model that is causing an issue (in the routes I guess), like where is that a problem ?

Here we have connection_id with serialization_alias set to conn_id. Hence, we can't use by_alias=True.

I didn't get that. Do you see an error ?

[rawwar]

I was talking about the usage of the model that is causing an issue (in the routes I guess), like where is that a problem ?

airflow/airflow/api_fastapi/core_api/routes/public/connections.py

Line 188 in 2f5cfbf

ConnectionBody(**patch_body.model_dump())

when we use by_alias=True, it throws the following error. Because, connection_id is a required field and because of serilization_alias, model_dump returns it as conn_id:

'{"detail":[{"type":"missing","loc":["connection_id"],"msg":"Field required","input":{"conn_id":"test_connection_id","conn_type":"test_type","description":null,"host":null,"login":null,"schema":null,"port":null,"password":null,"extra":"{\\"key\\": \\"var\\"}"},"url":"https://errors.pydantic.dev/2.10/v/missing"},{"type":"extra_forbidden","loc":["conn_id"],"msg":"Extra inputs are not permitted","input":"test_connection_id","url":"https://errors.pydantic.dev/2.10/v/extra_forbidden"}]}'

[rawwar]

I don't know. I don't think we need that in the base model yet. (edited: if the CI is happy with it lets do that)

I think its going to pass. Since, we don't have extra=forbid yet

After thinking about above, I think its irrelevant

[pierrejeambrun]

#46245 should fix unrelated test failures