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

TypeScript types for the azure-iot-provisioning-service library appear to be incorrect #1199

Open
mccolljr opened this issue Feb 13, 2024 · 8 comments
Labels

Comments

@mccolljr
Copy link

In the last hour I have encountered two scenarios where the TypeScript types are completely incorrect for the azure-iot-provisioning-service library.

This is the version I am using:

"azure-iot-provisioning-service": "^1.11.2"
  1. The createOrUpdateIndividualEnrollment method on the client takes an IndividualEnrollment object as input. The IndividualEnrollment object does not consider any of the fields optional, despite the fact that the endpoint will accept a very small subset of fields. Why is there not a separate input type which accurately reflects which fields are required?

  2. The createIndividualEnrollmentQuery method on the client returns a Query object. The user is then supposed to call the Query object's next method to receive result pages. The type annotations for this next method indicate that the returned object has a responseBody property of the form { items: Array<one | of | many | things> }, but the actual value returned from the API is the items array directly.

Based on the limited testing I have done, I am certain that there would be other areas where types are incorrect. Is this a matter of re-generating these types from the underlying API specification? What can be done to resolve this?

@mccolljr mccolljr added the bug label Feb 13, 2024
@anthonyvercolano
Copy link
Contributor

It could be that if you don't include every property in the update, those properties that aren't present will be deleted.

@anthonyvercolano
Copy link
Contributor

It's kind of bad because essentially it might force you to FIRST GET all the properties, then use what you got to setup the update. This is something that would NOT likely be fixed. It's more of a server side issue.

(Test this out. I could be wrong)

@mccolljr
Copy link
Author

If I were to obey the input type exactly, I would provide this:

await this.client.createOrUpdateIndividualEnrollment({
    attestation: {
        type: 'symmetricKey',
        symmetricKey: {
            primaryKey: '<secret>',
            secondaryKey: '<secret>'
        }
    },
    createdDateTimeUtc: 'pretty sure the server does not expect this from the user',
    deviceId: '<secret>',
    etag: 'this is not required, and MUST be omitted when creating',
    initialTwin: {
        tags: {
            count: 1,
            version: 1,
            metadata: {
                lastUpdated: new Date(),
                lastUpdatedVersion: 1
            },
            myOneTag: 'Was the rest of this really required just to set ONE tag? The answer, according to the server, is no.'
        },
        properties: {
            desired: {
                count: 1,
                version: 1,
                metadata: {
                    lastUpdated: new Date(),
                    lastUpdatedVersion: 1
                }
            }
        }
    },
    iotHubHostName: 'I am fairly certain that you should provide EITHER this field...',
    iotHubs: ['...OR this field, but not both'],
    lastUpdatedDateTimeUtc: 'I am pretty sure the server does not accept this value from the end user',
    provisioningStatus: 'enabled',
    registrationId: '<secret>',
    registrationState: undefined // I am _required_ to set this field to undefined explicitly, but I
                                 // would imagine that this field should never be updated based on a
                                 // user request.
});

If I lie to TypeScript, and only fill in this:

await this.client.createOrUpdateIndividualEnrollment({
    registrationId: params.uniqueId,
    deviceId: params.uniqueId,
    initialTwin: {
        tags: {
            ...params.additionalTags,
            tenant_id: params.tenantId
        }
    },
    attestation: {
        type: 'symmetricKey',
        symmetricKey: {
            primaryKey: params.primarySASKey,
            secondaryKey: params.secondarySASKey
        }
    },
    iotHubs: [params.iotHub],
    provisioningStatus: 'enabled'
} as DeepPartial<IndividualEnrollment> as IndividualEnrollment)

...I get exactly the record I expect back from the server.

@mccolljr
Copy link
Author

The API for creating / updating devices directly with an IoT hub is far more ergonomic than this. I struggle to understand why the API for creating / updating individual enrollments, which share a large amount of their shape with devices themselves, should be so much less ergonomic.

It could be that if you don't include every property in the update, those properties that aren't present will be deleted.

This is only true for the update operation. Creating a new individual enrollment does not carry any of this risk, and the server will reject your attempt to create an enrollment if you haven't set an etag and there are conflicts with an existing enrollment. The presence of etag is a surefire way to differentiate between the create and update cases.

Now, having said that - there are some fields on the IndividualEnrollment type that I can't imagine are ever accepted from the user. Things like createdDateTimeUTC and lastUpdatedDateTimeUTC appear to be server-managed fields. Maybe these should not appear in the input type, but certainly shouldn't be required if they do.

It's kind of bad because essentially it might force you to FIRST GET all the properties, then use what you got to setup the update. This is something that would NOT likely be fixed. It's more of a server side issue.

I expect this sort of flow for updates - I definitely understand the need. My primary complaint is that the create part of "create or update" is a fundamentally different operation, and there is no "original record" which can be provided.

@mccolljr
Copy link
Author

However, none of this addresses

The createIndividualEnrollmentQuery method on the client returns a Query object. The user is then supposed to call the Query object's next method to receive result pages. The type annotations for this next method indicate that the returned object has a responseBody property of the form { items: Array<one | of | many | things> }, but the actual value returned from the API is the items array directly.

This appears to be a plainly incorrect typing, as the values I receive from the API are in a totally different shape than the typing implies they should be.

@anthonyvercolano
Copy link
Contributor

(I don't really have interaction with the project anymore so hopefully we'll see team members comment.)

Indeed, there are some fields it makes no sense to utilize, and it was the update case I was trying to address.

I feel your pain.🤕

@mccolljr
Copy link
Author

(I don't really have interaction with the project anymore so hopefully we'll see team members comment.)

Indeed, there are some fields it makes no sense to utilize, and it was the update case I was trying to address.

I feel your pain.🤕

Ah - understood. I appreciate you taking the time to respond!

@mccolljr
Copy link
Author

mccolljr commented Mar 8, 2024

Perhaps interesting: The types from this library don't even match what is here (which is still largely incorrect): https://learn.microsoft.com/en-us/rest/api/iot-dps/service/individual-enrollment/create-or-update?view=rest-iot-dps-service-2021-06-01#request-body

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants