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

Adding support for nextCursor #8

Merged
merged 6 commits into from
Aug 16, 2023
Merged

Conversation

friendscottn
Copy link
Contributor

@friendscottn friendscottn commented Jul 25, 2023

Fixes #6

I tried the following approaches before settling on a fix:

  • Adding an additional parameter nextCursor to the api spec. This wasn't the greatest solution though because you end up needing to figure out how to customize the src/Apis/MP/US/OrdersApi.php::getAllOrdersRequest function to add special case handling for the nextCursor parameter and not send the other default parameters. It seems like avoiding special cases like this is better if you can when working on generated code like this.
  • Adding a new pseudo endpoint with a path parameter containing the full nextCursor querystring v3/orders/{nextCursor}. This was no good because you can't include special characters (?, &, etc) in a path parameter according to the openapi specs I was looking through.

I settled on a solution where I add the additional missing query string parameters contained within the nextCursor response to the existing /v3/orders endpoint.

This is a little annoying for the end user of this library because you need to break up the querystring formatted nextCursor that you receive and then feed it back into the same getAllOrders() to get the subsequent pages. It might be useful to include a helper or utility class in this library to make this easy for people. But this seems like a nice to have not really necessary.

One thing I don't love about this solution is that i had to set all of the parameters for the v3/orders endpoint in the schema-customizations.json. Ideally I could just set the additional missing parameters and these would get merged with the others for this particular problem. It might be a good idea to add resources/scehema-additions.json that uses array_merge_recursive under the hood to add to the schemas during schema customization. Just a thought though. This seems good enough to me for now

@jlevers
Copy link
Contributor

jlevers commented Jul 27, 2023

Thanks for this @friendscottn – I'm going to take a proper look at it next week.

@jlevers
Copy link
Contributor

jlevers commented Aug 15, 2023

Hey @friendscottn – sorry for the delay here.

This looks good, but I'd like to get the recursive schema merging solution you mentioned figured out before I merge this, because the schema corrections file is going to get messy fast without it.

I think the simplest solution here is to make array_merge_recursive_distinct check each array being merged for a __mode key, which can be set to append or overwrite (with overwrite as the default, if the __mode key isn't present). How does that sound to you?

@friendscottn
Copy link
Contributor Author

Hey @jlevers, Yeah getting the merge option working first seems like a good idea.
Your __mode solution sounds great to me.

@jlevers
Copy link
Contributor

jlevers commented Aug 15, 2023

Just realized there was an implicit ask in my response that may have been unclear. Are you down to implement support for the __mode key?

@friendscottn
Copy link
Contributor Author

Oh yeah np.
So starting on this it seems like the __mode key is not allowed on json arrays directly.

{
  "parameters": [
    "__mode": "append",
    {
      "name": "hasMoreElements",
      "in": "query",
      "description": "hasMoreElements",
      "required": false,
      "schema": {
        "type": "string"
      }
    }
  ]
}

Didn't think about that. We'd either have to move the __mode to an object (which feels kinda awkward):

{
  "parameters": [
    {
      "__mode": "append"
    },
    {
      "name": "hasMoreElements",
      "in": "query",
      "description": "hasMoreElements",
      "required": false,
      "schema": {
        "type": "string"
      }
    }
  ]
}

I'm feeling like the schema_additions.json file feels a little better. What do you think?

@friendscottn
Copy link
Contributor Author

Mwahh haha! You're too late @jlevers the schema-additions.json has been added!

Copy link
Contributor

@jlevers jlevers left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, good point about the issue of not being able to put the __mode key in the right place – your solution of the schema-additions.json file works for me! Thanks Scott :)

@jlevers jlevers merged commit f7630ce into highsidelabs:main Aug 16, 2023
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

Successfully merging this pull request may close these issues.

Support for nextCursor
3 participants