RideNoCo / drt-tds

0 stars 1 forks source link

String required properties can be empty #6

Closed asiridissa closed 10 months ago

asiridissa commented 12 months ago

Marking as required does not check for empty string values. It needs to define minLength property. ex: "street": { "type": "string", "minLength": 1 } It needs to do this for all required string properties.

keviniano commented 11 months ago

To clarify, is this just a one-time activity of inserting into the JSON schema { "type": "string", "minLength": 1 } for every attribute that in the old XSD file there are type="xsd:string" use="required" or type="xsd:string" minOccurs="1" tags?

That is to say, there's no need to discuss whether this needs to be done?

If it's just a straightforward insertion of text in the right places, I can take this on.

asiridissa commented 11 months ago

Not exactly. use="required" and minOccurs="1" also means it only needs to have the element or attribute in XML. Still allows to have empty strings. https://stackoverflow.com/questions/59885734/make-sure-string-is-not-empty-in-xml-validation-with-xsd

But we know some fields that we cannot allow empty strings. ex: tripTicketId, addressName, street, city etc.

I think we need to decide list of properties we need to put minLength=1 in JSON.

Maybe you could start with properties you think that should be mandatory? And then we all can contribute as we find new?

keviniano commented 11 months ago

I'd suspect that we don't want empty strings in any situation, but it sounds like the next task is to have a version of the XSD that highlights all the affected attributes so we can confirm that. I'll make that document and put it in Drive.

asiridissa commented 11 months ago

I think we agreed on doing changes in JSON schema. We are not going to use XML anymore. Swagger is also based on schema.json now.

asiridissa commented 11 months ago

here is a solution for this issue and also for nullable issue #5.

This commit contains the changes in new branch.

https://github.com/RideNoCo/drt-tds/commit/e6109e3e8ab8787d00a0b5315d3892e74a7dbade

I created 2 types extending string type, and used these new types in 2 string fields as POC. stringNullable & stringNotEmpty

Please take a look. If this solution is acceptable then, we need to

image

keviniano commented 11 months ago

I think we agreed on doing changes in JSON schema. We are not going to use XML anymore. Swagger is also based on schema.json now.

Correct, I'm not suggesting making any changes to the XSD file. Rather, I'm saying it's the file that holds the information about which fields need to be made required in the JSON schema.

keviniano commented 11 months ago

I went through the latest XSD to highlight all the fields that are required in one way or another (file here), and I personally don't see anywhere it would make sense to allow zero-length strings.

As far as whether it's better to use complex types vs writing out the validation rule each time, I'm not familiar enough with any JSON schema development best practices to have much of an opinion. Both seem roughly equivalently readable and concise. Complex types might be easier to maintain.

asiridissa commented 11 months ago

@keviniano I agree with you. Let's start with the highlighted fields in the file.

I also vote for the complex type.

keviniano commented 10 months ago

I've created PR https://github.com/RideNoCo/drt-tds/pull/17 for this issue. Can folks please review?