Fix bug for multiple required fields without annotation (#180)

* Add reproduction scenario

only the first field is marked as required

* Separate code paths

for `isRequiredBasedOnAnnotation` for improved readability

* Prevent "None" from being set as default

when type is `Option[String]`

* Simplify logic

branch:

Author: SamTheisens

src/main/scala/com/github/swagger/scala/converter/SwaggerScalaModelConverter.scala

@@ -132,7 +132,11 @@ class SwaggerScalaModelConverter extends ModelResolver(SwaggerScalaModelConverte
             // default values set in annotation leads to default values set in Scala constructor being ignored
             maybeDefault.foreach { default =>
               schemaProperties.get(propertyName).foreach { property =>
-                property.setDefault(default())
+                val defaultValue = default()
+                defaultValue match {
+                  case None =>
+                  case _ => property.setDefault(defaultValue)
+                }
               }
             }
           }
@@ -156,20 +160,20 @@ class SwaggerScalaModelConverter extends ModelResolver(SwaggerScalaModelConverte
               val requiredFlag = !isOptional && !hasDefaultValue
               if (!requiredFlag && schema.getRequired != null && schema.getRequired.contains(propertyName)) {
                 schema.getRequired.remove(propertyName)
-              } else if (requiredFlag && SwaggerScalaModelConverter.isRequiredBasedOnAnnotation) {
+              } else if (requiredFlag) {
                 addRequiredItem(schema, propertyName)
               }
             }
             case annotations => {
-              val annotationSetting = getRequiredSettings(annotations).headOption.getOrElse(false)
-              val required = if (isOptional || SwaggerScalaModelConverter.isRequiredBasedOnAnnotation) {
-                annotationSetting
+              val annotationRequired = getRequiredSettings(annotations).headOption.getOrElse(false)
+              if (SwaggerScalaModelConverter.isRequiredBasedOnAnnotation) {
+                setRequiredBasedOnAnnotation(schema, propertyName, annotationRequired)
               } else {
-                !hasDefaultValue
+                setRequiredBasedOnType(schema, propertyName, isOptional, hasDefaultValue, annotationRequired)
               }
-              if (required) addRequiredItem(schema, propertyName)
             }
           }
+
         }
         schema
       }
@@ -178,6 +182,29 @@ class SwaggerScalaModelConverter extends ModelResolver(SwaggerScalaModelConverte
     }
   }
 
+  private def setRequiredBasedOnAnnotation(
+      schema: Schema[_],
+      propertyName: String,
+      annotationSetting: Boolean
+  ): Unit = {
+    if (annotationSetting) addRequiredItem(schema, propertyName)
+  }
+
+  private def setRequiredBasedOnType(
+      schema: Schema[_],
+      propertyName: String,
+      isOptional: Boolean,
+      hasDefaultValue: Boolean,
+      annotationSetting: Boolean
+  ): Unit = {
+    val required = if (isOptional) {
+      annotationSetting
+    } else {
+      !hasDefaultValue
+    }
+    if (required) addRequiredItem(schema, propertyName)
+  }
+
   private def updateTypeOnItemsSchema(primitiveType: PrimitiveType, propertySchema: Schema[_]) = {
     val updatedSchema = correctSchema(propertySchema.getItems, primitiveType)
     propertySchema.setItems(updatedSchema)

src/test/scala/com/github/swagger/scala/converter/ModelPropertyParserTest.scala

@@ -169,7 +169,7 @@ class ModelPropertyParserTest extends AnyFlatSpec with BeforeAndAfterEach with M
     nullSafeSeq(model.value.getRequired) shouldBe empty
   }
 
-  it should "prioritize required as specified in annotation by default" in new PropertiesScope[ModelWOptionIntSchemaOverrideForRequired] {
+  it should "prioritize required as specified in annotation by default" in new PropertiesScope[ModelWOptionIntSchemaOverrideForRequired](true) {
     val requiredIntWithDefault = model.value.getProperties.get("requiredIntWithDefault")
     requiredIntWithDefault shouldBe an[IntegerSchema]
     requiredIntWithDefault.asInstanceOf[IntegerSchema].getDefault shouldEqual 5
@@ -186,6 +186,10 @@ class ModelPropertyParserTest extends AnyFlatSpec with BeforeAndAfterEach with M
       val annotatedOptionalIntWithSomeDefault = model.value.getProperties.get("annotatedOptionalIntWithSomeDefault")
       annotatedOptionalIntWithSomeDefault shouldBe an[IntegerSchema]
       annotatedOptionalIntWithSomeDefault.asInstanceOf[IntegerSchema].getDefault should be(5)
+
+      val annotatedOptionalStringWithNoneDefault = model.value.getProperties.get("annotatedOptionalStringWithNoneDefault")
+      annotatedOptionalStringWithNoneDefault shouldBe an[StringSchema]
+      annotatedOptionalStringWithNoneDefault.asInstanceOf[StringSchema].getDefault should be(null)
     }
 
     nullSafeSeq(model.value.getRequired).toSet shouldEqual Set("annotatedOptionalInt", "requiredInt")
@@ -211,11 +215,27 @@ class ModelPropertyParserTest extends AnyFlatSpec with BeforeAndAfterEach with M
       val annotatedOptionalIntWithSomeDefault = model.value.getProperties.get("annotatedOptionalIntWithSomeDefault")
       annotatedOptionalIntWithSomeDefault shouldBe an[IntegerSchema]
       annotatedOptionalIntWithSomeDefault.asInstanceOf[IntegerSchema].getDefault should be(5)
+
+      val annotatedOptionalStringWithNoneDefault = model.value.getProperties.get("annotatedOptionalStringWithNoneDefault")
+      annotatedOptionalStringWithNoneDefault shouldBe an[StringSchema]
+      annotatedOptionalStringWithNoneDefault.asInstanceOf[StringSchema].getDefault should be(null)
     }
 
     nullSafeSeq(model.value.getRequired).toSet shouldEqual Set("annotatedOptionalInt", "requiredInt", "annotatedRequiredInt")
   }
 
+  it should "consider fields that aren't optional required if `requiredBasedAnnotation == true`" in new PropertiesScope[
+    ModelWMultipleRequiredFields
+  ](
+    requiredBasedAnnotation = false
+  ) {
+    nullSafeSeq(model.value.getRequired).toSet shouldEqual Set("first", "second", "third")
+  }
+
+  it should "consider fields that aren't optional required" in new PropertiesScope[ModelWMultipleRequiredFields]() {
+    nullSafeSeq(model.value.getRequired).toSet shouldEqual Set("first", "second", "third")
+  }
+
   it should "process Model with Scala Option Long" in new PropertiesScope[ModelWOptionLong] {
     val optLong = model.value.getProperties().get("optLong")
     optLong should not be (null)

src/test/scala/models/ModelWMultipleRequiredFields.scala

@@ -0,0 +1,7 @@
+package models
+
+case class ModelWMultipleRequiredFields(
+    first: Double,
+    second: Double,
+    third: Double
+)

src/test/scala/models/ModelWOptionInt.scala

@@ -31,5 +31,6 @@ case class ModelWOptionIntSchemaOverrideForRequired(
     @Schema(description = "annotated default", defaultValue = "10") annotatedIntWithDefault: Int,
     @Schema(description = "should become required", required = true) annotatedOptionalInt: Option[Int],
     @Schema(description = "should not have default") annotatedOptionalIntWithNoneDefault: Option[Int] = None,
-    @Schema(description = "should have default") annotatedOptionalIntWithSomeDefault: Option[Int] = Some(5)
+    @Schema(description = "should have default") annotatedOptionalIntWithSomeDefault: Option[Int] = Some(5),
+    @Schema(description = "should not have a default") annotatedOptionalStringWithNoneDefault: Option[String] = None
 )