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

API mismatch with reality? #148

Closed
SoftwareApe opened this issue Mar 24, 2023 · 10 comments
Closed

API mismatch with reality? #148

SoftwareApe opened this issue Mar 24, 2023 · 10 comments

Comments

@SoftwareApe
Copy link

SoftwareApe commented Mar 24, 2023

Version in use

I'm using version 0.7.4 with the current version of Azure DevOps (which as far as I can tell is still the same API version 7.1.

Case 1

When using the pipeline_client to get a current pipeline I'm getting errors of the sort:

context: Full(Custom { kind: DataConversion, error: Error("missing field ``id``", line: 1, column: 455) }.

However looking inside the response an "id" field is present. So the error message seems wrong

Case 2

When using the runs_client on the same pipeline I'm getting this error:

context: Full(Custom { kind: DataConversion, error: Error("missing field ``finishedDate``", line: 1, column: 1723) }

finishedDateis indeed not part of the response. However this run isn't finished so I wouldn't expect it? I'm guessing this value should be optional and isn't.

Case 3

If I choose a run that has finished the response is ok.

Summary

I guess some of the API isn't covered correctly. This is especially an issue since I need unfinished runs from the API.

@SoftwareApe
Copy link
Author

I think Case #2 could be solved by replacing this diff in src/pipelines/models.rs:436

-    #[serde(rename = "finishedDate", with = "crate::date_time::rfc3339")]
-    pub finished_date: time::OffsetDateTime,
+    #[serde(rename = "finishedDate", with = "crate::date_time::rfc3339", skip_serializing_if = "Option::is_none")]
+    pub finished_date: Option<time::OffsetDateTime>,

However since this crate is auto-generated I won't add a PR.

@SoftwareApe
Copy link
Author

SoftwareApe commented Mar 24, 2023

I checked and the optionality of finishedDate is indeed not documented. Could this somehow be patched?

https://learn.microsoft.com/en-us/rest/api/azure/devops/pipelines/runs/get?view=azure-devops-rest-7.1

@SoftwareApe
Copy link
Author

Looking at the API for the pipelines GET:
https://learn.microsoft.com/en-us/rest/api/azure/devops/pipelines/pipelines/get?view=azure-devops-rest-7.1

It seems to be all there however I noticed the fields in the API definition are:

  • _links
  • configuration
  • folder
  • id
  • name
  • revision
  • url

However the response I get is:

  • _links
  • configuration
  • url
  • id
  • revision
  • name
  • folder

So I'm guessing we're looking for folder, find folder at the end. But then skip everything in between, and the next field id can't be found.

I guess the fix would be not to care for the order here. Is that possible?

@johnbatty
Copy link
Collaborator

Thanks very much for the detailed report!

For Case 2, as you suggest the issue is that the existing code treats Run::finishedDate as a required field. This was actually due to configuration in my spec patcher to try and eliminate some of the Option wrappers in returned structs. See here for more details. It had worked in my previous testing, but that was just because I hadn't tested with an InProgress run!
#6

I managed to repro the issue, and found that the result field is also missing when the status is InProgress, so have also made that optional.

@johnbatty
Copy link
Collaborator

For Case 1, the issue is not field ordering - the code is fine with the fields in any order.

Given the information that you have provided, I think the issue is with an id field elsewhere in the nested structs. Looking at the definitions, I see that the configuration field is of type PipelineConfiguration, and that has a repository field of type Repository that has id and type fields which are currently required. I am guessing that in your scenario the id must be missing from this value (possibly the type too?). It would be good if you could confirm, if you can still repro and look at the data.

I'll make a speculative fix here to make both of these fields optional, which should hopefully fix your scenario.

@SoftwareApe
Copy link
Author

For case 1 I can check on Monday, I'm not sure what the output was, I just didn't want to post the full response here, but I could add a redacted response maybe.

@johnbatty
Copy link
Collaborator

Yes - don't post any of your data here.
When you get the error, you should get the response data on the console. It will be a long single line. The error message gives you a location where the issue is, e.g. Error("missing field id", line: 1, column: 455). You should be able to copy/paste the data into an editor and look at that column position. It should be at the end of the object that is missing the id field. You might be able to figure out which structure field it is (in particular, whether it is the repository field).

@johnbatty
Copy link
Collaborator

I've just published a new release 0.7.5 that I think should fix both of your issues (assuming that my speculative fix works).
Please try it out and let me know.

@SoftwareApe
Copy link
Author

SoftwareApe commented Mar 27, 2023

@johnbatty Hi, I checked the response and the speculative fix on the pipelines indeed worked, the repository field is repository: Repository { id: None, type_: Some("gitHubEnterprise") }, I'm guessing this only happens with non-azure repos or something other that's unusual.

Getting an inProgress run (as well as a finished run) works, too.

Thank you!

@johnbatty
Copy link
Collaborator

Great news. I suspected that the Repository might look like that, and would explain why I hadn't seen it (I've only tested with pipelines that refer to ADO repos).

Please continue to raise any other issues you find.

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

No branches or pull requests

2 participants