Closed MarkKoz closed 6 years ago
You will need to pull from master to get the SemVer fixes in this repo too.
Thanks for the contribution. Much appreciated.
Should the tags be converted to a more sensible format as I have done, or should closely matching Steam's response be maintained?
Have you seen tags contain anything more than a string for the name of the tag? The JSON response makes each tag an object which makes me think other properties could be returned. But if it's only ever a string, your conversion is OK with me.
One which infers itemcount from the size of publishedFileIds. Alternatively, it could be made an optional argument. In that case, it would make the most sense for the default to be 0.
If itemcount
is truly a duplication of publishedFileIds.Count
, then I agree with making an overload where itemcount
is calculated implicitly.
One which takes only one ID and returns a single object instead of a list.
Thumbs up from me.
In reference to result in an object in publishedfiledetails, how should unsuccessful results be handled? Currently, most properties will be null on failure, but this isn't transparent to the client. I'm inclined to leave it as is because, based on what I have seen from the rest of the design, this wrapper does not seem to concern itself with notifying the client of failure. The only way in which this is done is though returning null.
Since the Steam Web API is so inconsistent with returning error and status codes, I never came up with a good, standard approach to notifying the users of an error. For example, a not found error from the Steam Web API might be a null response or a 404 or a 500 or a response containing a complex status code depending on which endpoint it is and who wrote the code at Valve.
Reading your suggestions makes me think that it might be a good idea to do the following. This is just spit-balling and thinking out loud so let me know what you think. I don't think the following should be accomplished on this PR, but you've definitely sparked a good discussion about this.
IsSuccess
boolean property to the SteamWebResponse
classErrorData
generic (K
) property to the SteamWebResponse
class where K
is a type that represents the unique error model of that endpoint (since it can be different per endpoint)IsSuccess
is true if ErrorData
is not null, false otherwiseThis would scale for all endpoints to standardize the approach of notifying a user of errors.
For your new method, the model that would represent the ErrorData
of your SteamWebResponse
might look like:
public class PublishedFileDetailsErrorModel
{
public PublishedFileDetailsResultCode Result { get; set; }
}
Where PublishedFileDetailsResultCode
is an enum of each possible value that we know about (looks like 1 is success and 9 is not found?) I can't find documentation on this so we might need to add as we go.
public enum PublishedFileDetailsResultCode
{
Success = 1,
NotFound = 9
}
This way, it's a consistent interface for consumers to check for errors and any relevant details across all methods of the library.
var playerSummaryResponse = await steamUser.GetPlayerSummaryAsync(123456);
if(!playerSummaryResponse.IsSuccess)
{
// Do something about it, check playerSummaryResponse.ErrorData for details to take action
}
What do you think?
Have you seen tags contain anything more than a string for the name of the tag? The JSON response makes each tag an object which makes me think other properties could be returned. But if it's only ever a string, your conversion is OK with me.
I haven't seen anything else, but I haven't looked at a huge sample of responses. I used a similar converter to retrieve a lot of workshop files and I never saw anything that isn't a tag end up in the resulting list. However, I acknowledge this endpoint isn't solely for workshop items - it's for "published files", but the documentation is vague on what that actually means. Here's what I've determined:
Perhaps, for now, a safe compromise would be to convert to a dictionary with values as a list:
"tags": [
{
"tag": "Custom"
},
{
"tag": "Training"
},
{
"other": "NotATag"
}
]
to
"tags": {
"tag": [
"Custom",
"Training"
],
"other": [
"NotATag"
]
}
This is still kind of ugly unfortunately.
If itemcount is truly a duplication of publishedFileIds.Count, then I agree with making an overload where itemcount is calculated implicitly.
The response is 400 if itemcount
is greater than publishedFileIds.Count
, hence the Debug.Assert(itemCount <= publishedFileIds.Count)
. If it's less than, items are cut off seemingly respecting the order in which the IDs were passed and the response is valid.
Reading your suggestions makes me think that it might be a good idea to do the following.
That looks promising. It provides an abstraction to the unique error models but still provides access to them if clients have a use for the specifics. That being said, I still think the aim should be to keep the error models as generic as possible; I don't like the sound of having error models tied to a specific endpoint unless there are some edge cases. Error models should inheirt from an interface or abstract class if it's determined it makes sense to do so.
I'm not familiar with most endpoints, but here are some things error models may include:
EResult
enum linked below.itemCount
> publishedFileIds.Count
, which is more specific than just a 400 code.publishedfileid
and result
, only pretend there's no result
.Where PublishedFileDetailsResultCode is an enum of each possible value that we know about (looks like 1 is success and 9 is not found?) I can't find documentation on this so we might need to add as we go.
You missed the link earlier. The enum is documented here. It's under Steamworks but states it is also used by the web API. I already had a nicely-formatted version for C# lying around. It's here.
Sorry, fixed the screwed up formatting on my comment above.
HTTP status codes
Agree. Easy to add and definitely useful to consumers.
Boolean indicative of the validity of the deserialised object.
Good idea on the schema validator, but it's tough to determine every valid response model especially since it's so different per endpoint and even per response within the same endpoint.
EResult
enum
What about endpoints that don't contain this in the JSON response on error? There are some that simply return null, or empty lists, or empty objects instead of a nice result code. I guess we could fall back on "unknown".
For example, GetPlayerSummaries
returns the following when the passed Steam ID is not found:
{
"response": {
"players": [
]
}
}
And GetUserStatsForGame
returns HTTP 400 when either SteamID
or AppID
are not found.
When parameters are incorrect...
I prefer to fail as soon as possible, so it might be a good idea to do input validation at the entry point to the method as a sort of business logic. For example, if we know that itemCount
should never exceed publishedFileIds.Count
, then we can throw an InvalidOperationException
or ArgumentOutOfRangeException
to immediately indicate the problem.
It might be possible for causes of failure to be determined from failed/partial deserialisation...
True, we could possibly create a custom enum which has all the values from the EResult
plus custom values that are defined specifically for this library. A concern here is that some values may only be applicable to specific endpoints.
Depends on PR babelshift/Steam.Models#11. If you want, I can make another commit with an updated dependency version of the package once that PR is merged. That way, the check will pass. I didn't do it this commit because I figured the version number would change anyway after that PR.
Good idea on the schema validator, but it's tough to determine every valid response model especially since it's so different per endpoint and even per response within the same endpoint.
That was my concern. However, at a glance, the technology seems to be pretty flexible/configurable. I think it's just a matter of how much time one wishes to invest in making all the schemas. It's something to explore but I don't think it'll be worth it in the end.
What about endpoints that don't contain this in the JSON response on error?
It doesn't have to be included for everything. As I said, there should be some sort of base class or interface and things will be built up from there (would make it easy for them all to have an HTTP status code). Anything that uses the enum would have a derived class that contains that member.
I prefer to fail as soon as possible
Absolutely agree. I don't know what I was thinking - late night I suppose. Exceptions are good for getting the obvious things out of the way.
I've merged babelshift/Steam.Models#11 and version 3.0.6
was pushed to nuget. You can update your reference and commit to this PR.
Thanks for the help. I'm going to open an issue with our discussion above so I don't forget to revisit it.
Here is Steam's documentation of the endpoint. Example responses are shown at the end.
These changes depend on the existence of
PublishedFileDetailsModel
andPublishedFileVisibility
in Steam.Models. They have been added in babelshift/Steam.Models#8. The version of the NuGet package dependency has not been changed.I hope the commit messages and diffs are enough of a description of the changes. Instead, I will discuss some concerns I have with my implementation.
GetPublishedFileDetailsAsync
.itemcount
from the size ofpublishedFileIds
. Alternatively, it could be made an optional argument. In that case, it would make the most sense for the default to be 0.result
in an object inpublishedfiledetails
, how should unsuccessful results be handled? Currently, most properties will benull
on failure, but this isn't transparent to the client. I'm inclined to leave it as is because, based on what I have seen from the rest of the design, this wrapper does not seem to concern itself with notifying the client of failure. The only way in which this is done is though returningnull
.That being said, here are some options (not mutually exclusive):
result
for success.GetPublishedFileDetailsAsync
's summary.EResult
enum to System.Models and mapresult
to it.PublishedFileDetailsModel
which returns a boolean indicative of the success of retrieving the details for that instance of the object.Example of a failed response (file was not found):
Example of a successful response: