Fix primary key always being marked as nullable in schema

[pmdevita]

Fixes #1227

A regression occurred with #1181 that made it so primary keys were always marked as nullable in a Schema. This fixes that regression and then also fixes the tests that were incorrectly expecting it that way.

I have no idea why you would lol, but if for some reason you had a primary key field that was marked as nullable on the model, then the Schema should respect that, so I added a new test to ensure that happens.

This will change the default behavior of the schema generation, but it just restores it to the correct pre-1.2.0 behavior for primary keys. For any users that want their primary key to be nullable/optional, they should be either setting the model field to null=True or marking the field as optional with fields_optional on their ModelSchema.

[vitalik]

Well there is a bit of controversy here...

when you create schema from model (with ModelSchema) and then want to use it as input payload you do not want to primary key field to be required... I was thinking about it - I do not yet see some perfect solution

[pmdevita]

I see what you mean, that could be troublesome for users that are using the same schema for a request and a response data type. But for those cases, they could add the primary key to fields_optional and get the same functionality. This PR will enable users to decide what they want on their schemas, but the current behavior does not allow for any choice.

In general, I think it's a bit odd to always translate primary keys as null when every other model field's setting is respected by ModelSchema. ModelSchema is otherwise consistent in translating your database models, so I think this could be surprising behavior to users.

[vitalik]

I would also try to play with Optional or readonly (which makes sense logically - automatic primary keys can be read, but cannot be set in most cases) I think pydantic now handles that

[pmdevita]

Sorry to ask, but could you provide a bit more context for what you mean here? How are you suggesting we use Optional? Also, as far as I can tell, there isn't a read only property for either Django or Pydantic models (although Django does have editable=False).

[vitalik]

@pmdevita ist's a part of OpenAPI spec - fields can be marked as readonly and I assume swagger UI f.e. will not let you pass it as payloads

https://swagger.io/docs/specification/data-models/data-types/

[pmdevita]

Sorry for the late reply. To try this out, I set the read only property manually on a schema I have in a hobby project. This is using Ninja 1.1.0.

Which looks correct on the API schema.

And looks correct on Swagger UI (before/after)

However, the immediate problem that results from this is that Pydantic doesn't actually understand that the field is read only, and validation fails during the request. Read only isn't supported by Pydantic and for reference, FastAPI gets around this by recommending separate request and response schemas, and the Ninja docs also do the same. The read only property also has poor support in client library generators, neither openapi-generator or the TypeScript client I use, openapi-client-axios support it. In theory, I agree this would be a big part of an ideal solution, but support is just not there, both from Pydantic and the client side.

Just to give a bit of context on my usecase as well, my workplace is using Ninja now (which despite a few rough edges, is a really lovely experience so thank you!) and we're following the recommended strategy of using different schemas for request and response types. The current behavior of marking all primary keys as nullable would mean that our client teams would have to do an unnecessary null check every time they wanted to use the corresponding field on a response. The schema would not really reflect the API, because the API will never respond null for a primary key.

Since read only isn't a viable solution, I agree that users should be able to make primary keys nullable so that it would be possible to reuse their schemas for request and response. But ModelSchema's default behavior has always been to follow your model first, and to allow customization and exceptions to be configured from there. Making primary keys nullable by default breaks this otherwise consistent pattern. Given all of this, I think the best option here is still to follow the model field by default, and allow users to set their primary keys as nullable with fields_optional, it's consistent but still gives flexibility to those who need it.

[Ksauder]

In regards to the readonly discussion, there may be a chance to fix it with some custom typing in pydantic. I don't think it fixes everything, as I'm not sure how you would know in the schema whether it's an incoming payload or outgoing resource. However, it could at least be Optional and create the proper json schema.

I have also taken a crack at fixing the optional primary_key issue. It's rolled into broader changes, but the short and sweet of it is a new parameter primary_key_optional to both ModelSchema.Meta and create_schema() in #1281 . This allows the current functionality to exist, or you can make it False to revert to pre 1.2.0. Doesn't necessarily deal with this whole issue, and maybe the regression is only a bug in which case I'll remove that extra param.

[pmdevita]

It may be possible to emulate proper readonly functionality once we have proper validation vs serialization separation #1139. OpenAPI ecosystem support for it is very weak though, so it might be best to offer it as an option alongside a simpler option for null vs nonnullable primary key.

I think your solution is definitely preferable to the current behavior. The ability to create a custom ModelSchema parent class with custom configuration would smooth over the issue of having to set all of your primary keys as nullable manually (as my current solution would make you do). I still think primary keys being nullable by default is surprising and inconsistently opinionated behavior, and I would recommend it to be toggled off by default. But, even if it is on by default, it should at least provide a path forward for everyone.