Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

if constant is optional, don't force to value #952

Merged
merged 7 commits into from
Jun 16, 2021
Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 11 additions & 0 deletions ChangeLog.md
Original file line number Diff line number Diff line change
@@ -1,5 +1,16 @@
# Change Log

### 2021-06-07 - 5.8.1

min Autorest core version: 3.3.0

min Modelerfour version: 4.19.1

**Bug Fixes**

- Fix optional properties with constant schemas. Now, properties that have constant schemas but are optional will not have the hardcoded constant value,
but will default to its `x-ms-client-default` or `None` #952

### 2021-05-17 - 5.8.0

min Autorest core version: 3.3.0
Expand Down
1 change: 0 additions & 1 deletion autorest/codegen/models/object_schema.py
Original file line number Diff line number Diff line change
Expand Up @@ -129,7 +129,6 @@ def fill_instance_from_yaml(self, namespace: str, yaml_data: Dict[str, Any], **k
# map of discriminator value to child's name
for children_yaml in yaml_data["discriminator"]["immediate"].values():
subtype_map[children_yaml["discriminatorValue"]] = children_yaml["language"]["python"]["name"]

if yaml_data.get("properties"):
properties += [
Property.from_yaml(p, has_additional_properties=len(properties) > 0, **kwargs)
Expand Down
2 changes: 1 addition & 1 deletion autorest/codegen/models/parameter.py
Original file line number Diff line number Diff line change
Expand Up @@ -173,7 +173,7 @@ def in_method_signature(self) -> bool:

@property
def in_method_code(self) -> bool:
return not (isinstance(self.schema, ConstantSchema) and self.location == ParameterLocation.Other)
return not (self.constant and self.location == ParameterLocation.Other)

@property
def implementation(self) -> str:
Expand Down
30 changes: 20 additions & 10 deletions autorest/codegen/models/property.py
Original file line number Diff line number Diff line change
Expand Up @@ -32,26 +32,36 @@ def __init__(
self.required: bool = yaml_data.get("required", False)
self.readonly: bool = yaml_data.get("readOnly", False)
self.is_discriminator: bool = yaml_data.get("isDiscriminator", False)
# this bool doesn't consider you to be constant if you are a discriminator
self.constant: bool = isinstance(self.schema, ConstantSchema) and not self.is_discriminator

if description:
self.description = description
else:
self.description = yaml_data["language"]["python"]["description"]

validation_map: Dict[str, Union[bool, int, str]] = {}
self.client_default_value = client_default_value

@property
def constant(self) -> bool:
Copy link
Contributor Author

Choose a reason for hiding this comment

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

decided to make constant and validation_map readonly properties to unclutter the main__init__ and because we don't set them later

# this bool doesn't consider you to be constant if you are a discriminator
# you also have to be required to be considered a constant
return (
isinstance(self.schema, ConstantSchema) and
self.required and
not self.is_discriminator
)

@property
def validation_map(self) -> Optional[Dict[str, Union[bool, int, str]]]:
retval: Dict[str, Union[bool, int, str]] = {}
if self.required:
validation_map["required"] = True
retval["required"] = True
if self.readonly:
validation_map["readonly"] = True
retval["readonly"] = True
if self.constant:
validation_map["constant"] = True
retval["constant"] = True
if self.schema.validation_map:
validation_map_from_schema = cast(Dict[str, Union[bool, int, str]], self.schema.validation_map)
validation_map.update(validation_map_from_schema)
self.validation_map = validation_map or None
self.client_default_value = client_default_value
retval.update(validation_map_from_schema)
return retval or None

@property
def escaped_swagger_name(self) -> str:
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -214,6 +214,7 @@ async def test_model_flattening_simple(self, client):
product_id = "123",
description = "product description",
max_product_display_name = "max name",
capacity="Large",
odata_value = "http://foo",
generic_value = "https://generic"
)
Expand All @@ -230,6 +231,7 @@ async def test_model_flattening_with_parameter_flattening(self, client):
product_id = "123",
description = "product description",
max_product_display_name = "max name",
capacity="Large",
odata_value = "http://foo"
)
simple_product.additional_properties = {} # Not the purpose of this test. This enables the ==.
Expand All @@ -238,6 +240,7 @@ async def test_model_flattening_with_parameter_flattening(self, client):
"123", # product_id
"product description", # description
"max name", # max_product_display_name
"Large", # capacity
None, # generic_value
"http://foo", # odata_value
)
Expand All @@ -252,6 +255,7 @@ async def test_model_flattening_with_grouping(self, client):
product_id = "123",
description = "product description",
max_product_display_name = "max name",
capacity="Large",
odata_value = "http://foo"
)
simple_product.additional_properties = {} # Not the purpose of this test. This enables the ==.
Expand All @@ -260,6 +264,7 @@ async def test_model_flattening_with_grouping(self, client):
product_id = "123",
description = "product description",
max_product_display_name="max name",
capacity="Large",
odata_value="http://foo",
name="groupproduct")

Expand Down
5 changes: 5 additions & 0 deletions test/vanilla/AcceptanceTests/test_model_flattening.py
Original file line number Diff line number Diff line change
Expand Up @@ -208,6 +208,7 @@ def test_model_flattening_simple(self, client):
product_id = "123",
description = "product description",
max_product_display_name = "max name",
capacity="Large",
odata_value = "http://foo",
generic_value = "https://generic"
)
Expand All @@ -223,6 +224,7 @@ def test_model_flattening_with_parameter_flattening(self, client):
product_id = "123",
description = "product description",
max_product_display_name = "max name",
capacity="Large",
odata_value = "http://foo"
)
simple_product.additional_properties = {} # Not the purpose of this test. This enables the ==.
Expand All @@ -231,6 +233,7 @@ def test_model_flattening_with_parameter_flattening(self, client):
"123", # product_id
"product description", # description
"max name", # max_product_display_name
"Large", # capacity
None, # generic_value
"http://foo", # odata_value
)
Expand All @@ -244,6 +247,7 @@ def test_model_flattening_with_grouping(self, client):
product_id = "123",
description = "product description",
max_product_display_name = "max name",
capacity="Large",
odata_value = "http://foo"
)
simple_product.additional_properties = {} # Not the purpose of this test. This enables the ==.
Expand All @@ -252,6 +256,7 @@ def test_model_flattening_with_grouping(self, client):
product_id = "123",
description = "product description",
max_product_display_name="max name",
capacity="Large",
odata_value="http://foo",
name="groupproduct")

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -164,47 +164,33 @@ def __init__(self, **kwargs):
class NoModelAsStringNoRequiredOneValueDefault(msrest.serialization.Model):
"""NoModelAsStringNoRequiredOneValueDefault.

Variables are only populated by the server, and will be ignored when sending a request.

:ivar parameter: Default value: "value1".
:vartype parameter: str
:param parameter:
Copy link
Contributor Author

Choose a reason for hiding this comment

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

swagger. Since this is an optional constant with an x-ms-client-default, we default to the client default.

The change in description shows me our descriptions can be improved, filed a separate issue for it here.

:type parameter: str
"""

_validation = {
"parameter": {"constant": True},
}

_attribute_map = {
"parameter": {"key": "parameter", "type": "str"},
}

parameter = "value1"

def __init__(self, **kwargs):
super(NoModelAsStringNoRequiredOneValueDefault, self).__init__(**kwargs)
self.parameter = kwargs.get("parameter", "value1")


class NoModelAsStringNoRequiredOneValueNoDefault(msrest.serialization.Model):
"""NoModelAsStringNoRequiredOneValueNoDefault.

Variables are only populated by the server, and will be ignored when sending a request.

:ivar parameter: Default value: "value1".
:vartype parameter: str
:param parameter:
Copy link
Contributor Author

Choose a reason for hiding this comment

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

swagger. This is an optional constant with no default, so we default to None.

The doc should also be improved here, so we show the one non-None value. This is included in the same docstring issue

Copy link
Contributor Author

Choose a reason for hiding this comment

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

add docstring fix to this PR

:type parameter: str
"""

_validation = {
"parameter": {"constant": True},
}

_attribute_map = {
"parameter": {"key": "parameter", "type": "str"},
}

parameter = "value1"

def __init__(self, **kwargs):
super(NoModelAsStringNoRequiredOneValueNoDefault, self).__init__(**kwargs)
self.parameter = kwargs.get("parameter", None)


class NoModelAsStringNoRequiredTwoValueDefault(msrest.serialization.Model):
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -176,47 +176,33 @@ def __init__(self, *, parameter: Union[str, "ModelAsStringRequiredTwoValueNoDefa
class NoModelAsStringNoRequiredOneValueDefault(msrest.serialization.Model):
"""NoModelAsStringNoRequiredOneValueDefault.

Variables are only populated by the server, and will be ignored when sending a request.

:ivar parameter: Default value: "value1".
:vartype parameter: str
:param parameter:
:type parameter: str
"""

_validation = {
"parameter": {"constant": True},
}

_attribute_map = {
"parameter": {"key": "parameter", "type": "str"},
}

parameter = "value1"

def __init__(self, **kwargs):
def __init__(self, *, parameter: Optional[str] = "value1", **kwargs):
super(NoModelAsStringNoRequiredOneValueDefault, self).__init__(**kwargs)
self.parameter = parameter


class NoModelAsStringNoRequiredOneValueNoDefault(msrest.serialization.Model):
"""NoModelAsStringNoRequiredOneValueNoDefault.

Variables are only populated by the server, and will be ignored when sending a request.

:ivar parameter: Default value: "value1".
:vartype parameter: str
:param parameter:
:type parameter: str
"""

_validation = {
"parameter": {"constant": True},
}

_attribute_map = {
"parameter": {"key": "parameter", "type": "str"},
}

parameter = "value1"

def __init__(self, **kwargs):
def __init__(self, *, parameter: Optional[str] = None, **kwargs):
super(NoModelAsStringNoRequiredOneValueNoDefault, self).__init__(**kwargs)
self.parameter = parameter


class NoModelAsStringNoRequiredTwoValueDefault(msrest.serialization.Model):
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -454,6 +454,7 @@ async def post_flattened_simple_product(
product_id: str,
description: Optional[str] = None,
max_product_display_name: Optional[str] = None,
capacity: Optional[str] = "Large",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

as you'll notice, the default for optional constants in the method signature is a value. This has to do with this issue. Still need to find a good non-breaking way around this

generic_value: Optional[str] = None,
odata_value: Optional[str] = None,
**kwargs: Any
Expand All @@ -468,6 +469,8 @@ async def post_flattened_simple_product(
:type description: str
:param max_product_display_name: Display name of product.
:type max_product_display_name: str
:param capacity: Capacity of product. For example, 4 people.
:type capacity: str
:param generic_value: Generic URL value.
:type generic_value: str
:param odata_value: URL value.
Expand All @@ -485,6 +488,7 @@ async def post_flattened_simple_product(
product_id=product_id,
description=description,
max_product_display_name=max_product_display_name,
capacity=capacity,
generic_value=generic_value,
odata_value=odata_value,
)
Expand Down Expand Up @@ -565,6 +569,7 @@ async def put_simple_product_with_grouping(
product_id=_product_id,
description=_description,
max_product_display_name=_max_product_display_name,
capacity=capacity,
generic_value=_generic_value,
odata_value=_odata_value,
)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -159,8 +159,6 @@ def __init__(self, **kwargs):
class FlattenParameterGroup(msrest.serialization.Model):
"""Parameter group.

Variables are only populated by the server, and will be ignored when sending a request.

All required parameters must be populated in order to send to Azure.

:param name: Required. Product name with value 'groupproduct'.
Expand All @@ -175,8 +173,8 @@ class FlattenParameterGroup(msrest.serialization.Model):
:type description: str
:param max_product_display_name: Display name of product.
:type max_product_display_name: str
:ivar capacity: Capacity of product. For example, 4 people. Default value: "Large".
:vartype capacity: str
:param capacity: Capacity of product. For example, 4 people.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

swagger. Since current behavior of enum is modelAsString defaults to False, and required defaults to False, this param is an optional constant with no default value

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ask tim about whether modelAsString defaults to false or true

:type capacity: str
:param generic_value: Generic URL value.
:type generic_value: str
:param odata_value: URL value.
Expand All @@ -186,7 +184,6 @@ class FlattenParameterGroup(msrest.serialization.Model):
_validation = {
"name": {"required": True},
"product_id": {"required": True},
"capacity": {"constant": True},
}

_attribute_map = {
Expand All @@ -200,15 +197,14 @@ class FlattenParameterGroup(msrest.serialization.Model):
"odata_value": {"key": "@odata\\.value", "type": "str"},
}

capacity = "Large"

def __init__(self, **kwargs):
super(FlattenParameterGroup, self).__init__(**kwargs)
self.name = kwargs["name"]
self.simple_body_product = kwargs.get("simple_body_product", None)
self.product_id = kwargs["product_id"]
self.description = kwargs.get("description", None)
self.max_product_display_name = kwargs.get("max_product_display_name", None)
self.capacity = kwargs.get("capacity", None)
self.generic_value = kwargs.get("generic_value", None)
self.odata_value = kwargs.get("odata_value", None)

Expand Down Expand Up @@ -291,8 +287,6 @@ def __init__(self, **kwargs):
class SimpleProduct(BaseProduct):
"""The product documentation.

Variables are only populated by the server, and will be ignored when sending a request.

All required parameters must be populated in order to send to Azure.

:param product_id: Required. Unique identifier representing a specific product for a given
Expand All @@ -303,8 +297,8 @@ class SimpleProduct(BaseProduct):
:type description: str
:param max_product_display_name: Display name of product.
:type max_product_display_name: str
:ivar capacity: Capacity of product. For example, 4 people. Default value: "Large".
:vartype capacity: str
:param capacity: Capacity of product. For example, 4 people.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Same param as above

:type capacity: str
:param generic_value: Generic URL value.
:type generic_value: str
:param odata_value: URL value.
Expand All @@ -313,7 +307,6 @@ class SimpleProduct(BaseProduct):

_validation = {
"product_id": {"required": True},
"capacity": {"constant": True},
}

_attribute_map = {
Expand All @@ -325,11 +318,10 @@ class SimpleProduct(BaseProduct):
"odata_value": {"key": "details.max_product_image.@odata\\.value", "type": "str"},
}

capacity = "Large"

def __init__(self, **kwargs):
super(SimpleProduct, self).__init__(**kwargs)
self.max_product_display_name = kwargs.get("max_product_display_name", None)
self.capacity = kwargs.get("capacity", None)
self.generic_value = kwargs.get("generic_value", None)
self.odata_value = kwargs.get("odata_value", None)

Expand Down
Loading