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

Decimal fields conveyed as strings in the REST API #6338

Closed
emersonfelipesp opened this issue May 4, 2021 · 6 comments
Closed

Decimal fields conveyed as strings in the REST API #6338

emersonfelipesp opened this issue May 4, 2021 · 6 comments
Assignees
Labels
status: accepted This issue has been accepted for implementation type: bug A confirmed report of unexpected behavior in the application
Milestone

Comments

@emersonfelipesp
Copy link
Contributor

NetBox version

v2.11.2

Python version

3.9

Steps to Reproduce

The HTTP GET response returns "vcpu" argument as string, but it should be a float, as requested on Issue #5975.

Steps to reproduce:

  1. Make HTTP GET request to /api/virtualization/virtual-machines/{id}/

HTTP GET Request

URI: https://demo.netbox.dev/api/virtualization/virtual-machines/543/

Headers:
Authorization: Token 977e9fb09fbfb2aa67ae06f90655b52edcee01e5
Host: demo.netbox.dev
User-Agent: PostmanRuntime/7.28.0
Accept: /
Accept-Encoding: gzip, deflate, br
Connection: keep-alive
Content-Type: application/json

Expected Behavior

HTTP GET Response

{
    "id": 543,
    "url": "https://demo.netbox.dev/api/virtualization/virtual-machines/543/",
    "display": "vcpu is string, not float",
    "name": "vcpu is string, not float",
    "status": {
        "value": "active",
        "label": "Active"
    },
    "site": null,
    "cluster": {
        "id": 9,
        "url": "https://demo.netbox.dev/api/virtualization/clusters/9/",
        "display": "DO-AMS3",
        "name": "DO-AMS3"
    },
    "role": null,
    "tenant": null,
    "platform": null,
    "primary_ip": null,
    "primary_ip4": null,
    "primary_ip6": null,
    "vcpus": 10.00,
    "memory": 8196,
    "disk": 50,
    "comments": "",
    "local_context_data": null,
    "tags": [],
    "custom_fields": {},
    "config_context": {},
    "created": "2021-05-04",
    "last_updated": "2021-05-04T02:19:09.726988Z"
}

Observed Behavior

HTTP GET Response

{
    "id": 543,
    "url": "https://demo.netbox.dev/api/virtualization/virtual-machines/543/",
    "display": "vcpu is string, not float",
    "name": "vcpu is string, not float",
    "status": {
        "value": "active",
        "label": "Active"
    },
    "site": null,
    "cluster": {
        "id": 9,
        "url": "https://demo.netbox.dev/api/virtualization/clusters/9/",
        "display": "DO-AMS3",
        "name": "DO-AMS3"
    },
    "role": null,
    "tenant": null,
    "platform": null,
    "primary_ip": null,
    "primary_ip4": null,
    "primary_ip6": null,
    "vcpus": "10.00",
    "memory": 8196,
    "disk": 50,
    "comments": "",
    "local_context_data": null,
    "tags": [],
    "custom_fields": {},
    "config_context": {},
    "created": "2021-05-04",
    "last_updated": "2021-05-04T02:19:09.726988Z"
}
@emersonfelipesp emersonfelipesp added the type: bug A confirmed report of unexpected behavior in the application label May 4, 2021
@emersonfelipesp emersonfelipesp changed the title API returning vcpu as string, not float on /api/virtualization/virtual-machines/{id} API returning vcpu as string, not float (/api/virtualization/virtual-machines/{id}). May 4, 2021
@jeremystretch
Copy link
Member

I'm not sure why these values are getting quoted. As far as I can tell the serializer field is defined correctly. Could be an issue with DRF itself.

>>> from virtualization.api.serializers import VirtualMachineSerializer
>>> VirtualMachineSerializer().fields['vcpus']
DecimalField(allow_null=True, decimal_places=2, label='VCPUs', max_digits=6, min_value=0.01, required=False)

@jeremystretch jeremystretch added the status: under review Further discussion is needed to determine this issue's scope and/or implementation label May 4, 2021
@jeremystretch
Copy link
Member

Ok, yes, this seems to be intentional behavior by DRF: See the COERCE_DECIMAL_TO_STRING setting. (This issue also lends some context.) It seems to hinge on the difference between decimal and float types.

While I recognize that precision isn't likely a concern for this specific use case, I'm hesitant to deviate from the standard implementation. I can see this becoming quite muddy for values such as cable lengths.

@jeremystretch jeremystretch changed the title API returning vcpu as string, not float (/api/virtualization/virtual-machines/{id}). Decimal fields conveyed as strings in the REST API May 4, 2021
@jeremystretch
Copy link
Member

This also affects latitude and longitude on the dcim.Site model:

    "latitude": "12.345000",
    "longitude": "12.345000",

@jeremystretch
Copy link
Member

I'm not sure it's fair to classify this as a bug, since it is technically intended behavior, though I do think we're okay to change it. Let's tag this for v2.12.

@jeremystretch jeremystretch added status: accepted This issue has been accepted for implementation and removed status: under review Further discussion is needed to determine this issue's scope and/or implementation labels May 5, 2021
@jeremystretch jeremystretch added this to the v2.12 milestone May 5, 2021
@ziggekatten
Copy link

In general, I have an opinion of never expect specific data types in responses. Given all APIs out there one thing that I have discovered is that you have to catch and format data anyway before processing. To be honest, life would be easier if everything was strings, and the processing of responses handle convversions.

But thats me, always encapsulating stuff in str(), int(), float() and so on...

@emersonfelipesp
Copy link
Contributor Author

In general, I have an opinion of never expect specific data types in responses. Given all APIs out there one thing that I have discovered is that you have to catch and format data anyway before processing. To be honest, life would be easier if everything was strings, and the processing of responses handle convversions.

But thats me, always encapsulating stuff in str(), int(), float() and so on...

Although your opinion makes sense and I had to do exactly what you said, this specific behavior in Netbox is very different from the rest of the system, so would be great to make things working as expected, as the system is so consistent.

@jeremystretch jeremystretch self-assigned this May 10, 2021
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Aug 9, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
status: accepted This issue has been accepted for implementation type: bug A confirmed report of unexpected behavior in the application
Projects
None yet
Development

No branches or pull requests

3 participants