soundcloud / api

A public repo for our Developer Community to engage about bugs and feature requests on our Public API
147 stars 23 forks source link

Questions regarding deprecated fields in the new Track object (Blog Post) #43

Closed mgoodfellow closed 3 years ago

mgoodfellow commented 3 years ago

Hi,

Regarding this blog post: https://developers.soundcloud.com/blog/soundclouds-new-api-track-object

embeddable_by

We currently use the embeddable_by and make sure to check for "all". It was my understanding that historically you could stop your track being embedded on other sites via the SoundCloud Widget? Is this no longer the case / was it ever the case?

If this is always "all" then we can simply stop checking this field and we have no issues with the deprecation.

secret_token

We use this field to allow users to choose to embed private tracks on our site if they wish. We then use this field when building up the embed code to pass into the SC Widget.

I see there is a replacement for secret_uri - can we get the form of this URI as an example? We could read the URI to find the secret token and use this in our existing implementation, however, our preference would be to not deprecate this field if at all possible.

Our reasoning behind this is that the SC Widget target URL is built up with a number of options, "show_comments", "show_user" etc. and one of these options is "secret_token". By removing direct access to this field in the Track Object returned via the API, you will make a integration of your API rely on parsing the "secret_uri" to access the secret_token for use elsewhere, which in turn could lead to brittle integrations if, for example, any formatting is changed in the URI (ordering of params etc.)

Many thanks,

Mike

mgoodfellow commented 3 years ago

I see that secret_uri is already returned in existing requests, and is not a new addition. Therefore we have updated our integrations to be compatible with the above deprecations, and to parse secret_uri instead as these changes are rolling out mid-next week.

For now we will just ignore embeddable_by and assume all tracks are embeddable (it might well have always been returning "all")

rahul-sc commented 3 years ago

Hey @mgoodfellow thank you for your feedback. Definitely helps to know how you're using it. We're discussing this internally. Your original notes make sense. We'll circle back with a proposition in the next few days. For now, we're only gathering feedback on our deprecated list and not yet deleting the fields.

mgoodfellow commented 3 years ago

Hi @rahul-sc

Apologies, I misread the blog post and thought these changes were deploying from the 31st March - my mistake!

Just to add, we were also dependent on the user_id field, in fact using in preference to the user object where we only needed this value. The change to use the nested user object is simple enough and we have also done this to future proof ourselves, but I would expect many other integrations are probably dependent on the user_id field, and may have also used it as preference. I imagine it is a slightly dangerous field to deprecate as it is "core data" and it would be hard to know all consumers without versioning the API instead for a safe deprecation.

mgoodfellow commented 3 years ago

Actually, even if a consumer isn't actually using the user_id field, it is a not-nullable integer value in the existing contract. Whereas strings can be null in JSON and deprecation of these would lead to missing data, you would be breaking many consumers of your API where they are expecting a certain contract, as that contract is now broken.

In our case, our JSON parser would explode as a not-nullable value of int which is expected is no longer present and therefore the payload cannot be deserialized. We are removing this from the contract on our side as part of this discussion, but a change to the contract such as this could have unintended consequences for consumers even if they are not using the data field you deprecated.

Other potential problem fields would be:

rahul-sc commented 3 years ago

Regarding user_id, this is only being removed from the top-level track entity. The field will still be accessible in th user object in the model. I think we didn't clarify this in the email but I hope this clarifies it.

For original_content_size and last_modified, these are fields that we're hoping to deprecate and delete in the near term. So we may not address any improvements to it for the time being.

mgoodfellow commented 3 years ago

The issue is not the fact the data exists elsewhere, the problem exists in a breaking change to a API contract.

The existing contract for the Track object says there is a property called user_id that is an integer - this field is not optional (nullable) in the existing contract.

When we deserialize this data into a data structure, we expect that field to exist, if it does not exist, my JSON parser will throw an exception to effectively say, it is not possible to represent the received data into the data structure defined using the given contract

As a consumer, I might not be using that value, but my data structure defines this and therefore it would be an error for it not to be returned.

For dynamically typed languages such as JS etc, this is less of an issue, but in statically typed languages (C# etc) we define the data structures and their types to deserialize to.

My concern is that removing expected not-nullable fields from this contract, or any contract on the API would cause many consumers to break unexpectedly.

For example: https://github.com/haythem/SoundCloud.NET/blob/master/SoundCloud.NET/Track.cs#L48

I just found this library on GitHub - we do not use it ourselves, but anyone consuming this library would not longer be able to parse or deserialise your returned data into the data structure expected as the contract has changed.

I would strongly suggest to not deprecated any fields, but if absolutely needed, then generally nullable / "Optional" fields can be removed as they would not break deserialisation of consumers.

rahul-sc commented 3 years ago

Hey @mgoodfellow

In C#, you should be able to mark integers as nullable. Would prescribe this to decouple dependencies from data model revisions further. I'd recommend updating the SDK you use to better handle this. For now, we need to remove this field. But your feedback is well received and will be considered for future updates.

public int? UserId { get; set; }
mgoodfellow commented 3 years ago

Hi @rahul-sc,

The https://github.com/haythem/SoundCloud.NET package is not mine to maintain, it was just the first one I found with a quick cursory search to illustrate the issue. My larger concern is the wide reaching ramifications of a change like this without versioning the API for all the reasons I have explained. I'm not aware of your consumer/usage statistics, but the breakages could be wide reaching.

I have shared my concerns and that is all I can do. It does make me worry about missing a blog post one day and having an overnight collapse of our integration with SoundCloud. I would argue the level of impact this change could have would be worth emailing all active users of ClientIds just in case they have missed the news or the blog post, similar to what was done with the API v2 endpoint restrictions.

Would prescribe this to decouple dependencies from data model revisions further. I'd recommend updating the SDK you use to better handle this. I'd recommend updating the SDK you use to better handle this.

I think this is a dangerous statement. At the end of the day, you as the API provider issue a contract that the consumer can rely on.

In your latest blog post - https://developers.soundcloud.com/blog/soundclouds-new-api-track-object - you explicitly define what properties are Optional (Option[]).

Let us consider "duration": Int

Your contract explicitly says that Duration is a property which has an Integer value and it is not optional. The contract explicity defines it as always having a valid integer value. A null is not an integer. If I was to define my contract as:

int? duration

Then I am not adhering to your contract. Instead I'm saying that duration might be missing.

We rely on a contract to tell us what data we can expect to find or not to find. Simply marking all properties as nullable doesn't help, instead that just moves the problem up the chain. So now consider one day you remove duration from the contract, OK - the JSON deserialisation didn't fail, but now a component further down will fail as it is expecting a value, but the value is missing.

Nonetheless, thank you for the dialogue and listening to my concerns.

Mike

mgoodfellow commented 3 years ago

Hi,

Just a thought in addition, is there a mailing list we can use for these changes? Your blog doesn't provide a subscription feature (that I can find?), plus not all the articles are as important as this one, and twitter is noisy in general and requires manual checking.

I do understand the need for breaking changes to be made, my fear here is the fact I could have easily missed this change.

It would be great if as API consumers we had some mailing list to be advised of extremely important changes which require action, along with a timeline of the changes and reminders (we are all human!)

This would definitely help put my mind at ease as to being fully notified of future changes.

Many thanks,

Mike

alexweinstein commented 3 years ago

I totally agree with @mgoodfellow. Last night user_id (or lack of user_id) crashed everything. Once I figured it out the fix was quick.

The fact that this repo exists, and there is a two-way conversation about it, is so much better than the black hole it used to be. However, there's still room for communication to improve when the API changes.

Versioning the API would be my vote, mailing list would be my second choice. Either way, make a much bigger deal for any upcoming changes. Give us a clear heads up. Make it simple.

Thanks.

rahul-sc commented 3 years ago

Thanks for all your notes here. We appreciate having a direct channel of comms with our users.

Versioning the API is something we're definitely looking into and want to enable soon. We're finding ourselves that there are some unavoidable breaking changes. This is so that any breaking changes can be fully avoided in the future. Please bear with us as we try to improve the API Platform :]

We're going to also explore putting in place a mailing list to subscribe to blog posts as emails, per the feedback mentioned here.