networkoptix / nx_open

NetworkOptix open-source components used to build Powered-by-Nx products including Desktop Client for Network Optix Video Management Platform.
https://networkoptix.com
49 stars 19 forks source link

isBuildPublished() use of JSON null converted to int tested against >= 0 #2

Closed ptr727 closed 2 years ago

ptr727 commented 2 years ago

I am updating the logic in my app to follow the same logic for determining when a release is stable or pre-release.

I was looking at the logic in selectVmsRelease(), canReceiveUnpublishedBuild(), and isBuildPublished() in nx_open\vms\libs\nx_vms_update\src\nx\vms\update\releases_info.cpp.

struct NX_VMS_UPDATE_API ReleaseInfo
{
    nx::vms::api::SoftwareVersion version;
    Product product;
    int protocol_version = 0;
    PublicationType publication_type = PublicationType::release;
    qint64 release_date = 0;
    int release_delivery_days = 0;
};
#define ReleaseInfo_Fields \
    (version) \
    (product) \
    (protocol_version) \
    (publication_type) \
    (release_date) \
    (release_delivery_days)
{
    "packages_urls": [
        "https://updates.networkoptix.com/default",
        "http://beta.networkoptix.com/beta-builds/default"
    ],
    "releases": [
        {
            "product": "vms",
            "version": "5.0.0.35270",
            "protocol_version": 5002,
            "publication_type": "release",
            "release_date": null,
            "release_delivery_days": null
        },
        {
            "product": "vms",
            "version": "4.2.0.32840",
            "protocol_version": 4201,
            "publication_type": "release",
            "release_date": "1620848664362",
            "release_delivery_days": 30
        }
    ]
}
bool isBuildPublished(const ReleaseInfo& releaseInfo)
{
    return releaseInfo.release_date > 0 && releaseInfo.release_delivery_days >= 0;
}

The JSON for release_date is as a "" or null, while the release_delivery_days is int or null.

I am not 100% sure how your JSON C++ parser coverts the null to an int, and I am assuming it convert to default 0.
If that is the case, releaseInfo.release_delivery_days >= 0 will always be true, unless the JSON sets the value explicitly as a negative value, and I have yet to see any JSON that is not a quoted string int64 number (epoch milliseconds) or a null.

Am I misunderstanding the logic, or can I ignore releaseInfo.release_delivery_days >= 0?

nxmeta commented 2 years ago

@ptr727 I haven't checked this with devs. But what if releaseInfo.release_date = 0 or releaseInfo.release_delivery_days < 0?

I think when designing this we left some flexibility in terms of how releases can be published and how our client reacts to them.

When you say you've never seen this -- do you mean our prod JSON files never had such a state? Though I'm not sure if they had such a state or not, we may want to use this in the future. So I would recommend replicating this logic.

ptr727 commented 2 years ago

Thank you for replying.

When I said "I have yet to see", I mean in the JSON files being produced, i.e. I have never seen any negative values, just "" or null or 0.

Per your suggestion I'll duplicate the logic, in anticipation of a negative value possibly being used.