Skip to content

feat: Add and resolve query projection for products and specs respectively#97

Merged
rbell517 merged 26 commits intoni:masterfrom
Shri2109:users/shriram/fix-projection
Mar 4, 2025
Merged

feat: Add and resolve query projection for products and specs respectively#97
rbell517 merged 26 commits intoni:masterfrom
Shri2109:users/shriram/fix-projection

Conversation

@Shri2109
Copy link
Contributor

@Shri2109 Shri2109 commented Feb 27, 2025

What does this Pull Request accomplish?

  • Adds projection in the query products request and includes a separate response model, where all the fields are optional.
  • Alters the request and response models for Products client, to resolve some inconsistencies.
  • Includes a response model for query specs where all the fields of the model are made optional.

Why should this Pull Request be merged?

  • Projection is something there in the actual backend service but missing in the product client. It has been included as a feature here.
  • The projection for query specs works only if all the fields are optional in the response model. Previously, few of the fields were mandatory.
  • The request models for Products Client were previously inconsistent. Those were fixed here, and this might be a breaking change.

What testing has been done?

  • Integration tests are included.
  • Manual testing has been done using a py script.

@SSSantosh18 SSSantosh18 self-requested a review February 27, 2025 06:15
@Shri2109 Shri2109 changed the title fix: Add and resolve query projection for products and specs respectively breaking: Add and resolve query projection for products and specs respectively Feb 27, 2025
@Shri2109 Shri2109 changed the title breaking: Add and resolve query projection for products and specs respectively fix: Add and resolve query projection for products and specs respectively Feb 27, 2025
@SSSantosh18 SSSantosh18 marked this pull request as ready for review February 27, 2025 14:44
@SSSantosh18
Copy link
Contributor

@rbell517 as changes in both Product and Spec client were related to projection and model fixes, they are breaking changes and we have combined them into a single PR. Let us know if there are any concerns with it.

Also, is there any specific process to do a major version bump? The angular commit documentation guideline related to schematic versioning in the contributing doc didn't mention anything about this.

@SSSantosh18
Copy link
Contributor

SSSantosh18 commented Feb 27, 2025

@rbell517, as we have created different models for Query response and Update request, @santhoshramaraj was suggesting if we could expose a method to map and create a update request model from query response so that it will be easy for the users

Maybe something like this within the existing model class?

class UpdateProductRequest():
    def from(ProductResponse product): UpdateProductRequest
        update_request = ....logic to map model....
        return update_request

Please share your thoughts

@rbell517 rbell517 changed the title fix: Add and resolve query projection for products and specs respectively feat: Add and resolve query projection for products and specs respectively Feb 28, 2025
@rbell517
Copy link
Collaborator

@rbell517 as changes in both Product and Spec client were related to projection and model fixes, they are breaking changes and we have combined them into a single PR. Let us know if there are any concerns with it.

Also, is there any specific process to do a major version bump? The angular commit documentation guideline related to schematic versioning in the contributing doc didn't mention anything about this.

@SSSantosh18 @shri2k2
To do a major version bump you add a trailing paragraph to your commit message starting with BREAKING CHANGE: that then describes the breaking change. It will cause the version bump to be major and add the breaking changes to the changelog. I will take an action to update the contributing docs to point to conventional commits instead of the angular page.

@rbell517
Copy link
Collaborator

@rbell517, as we have created different models for Query response and Update request, @santhoshramaraj was suggesting if we could expose a method to map and create a update request model from query response so that it will be easy for the users

Maybe something like this within the existing model class?

class UpdateProductRequest():
    def from(ProductResponse product): UpdateProductRequest
        update_request = ....logic to map model....
        return update_request

Please share your thoughts

@SSSantosh18
I like this idea but lets save it for another PR. As stated in the results PR we will need to be careful when doing this w.r.t. workspace updates because they affect what permissions the update is required to have merely based on the presence of the workspace field in the request regardless of whether the workspace is actually changed. If you copy all of the fields you'll end up changing the required permissions unnecessarily. My suggestion in that PR was to add a allow_workspace_update field (default false) when doing a mapping like this to control whether to map over the workspace or not.

@Shri2109
Copy link
Contributor Author

Shri2109 commented Mar 3, 2025

@rbell517 as changes in both Product and Spec client were related to projection and model fixes, they are breaking changes and we have combined them into a single PR. Let us know if there are any concerns with it.
Also, is there any specific process to do a major version bump? The angular commit documentation guideline related to schematic versioning in the contributing doc didn't mention anything about this.

@SSSantosh18 @shri2k2 To do a major version bump you add a trailing paragraph to your commit message starting with BREAKING CHANGE: that then describes the breaking change. It will cause the version bump to be major and add the breaking changes to the changelog. I will take an action to update the contributing docs to point to conventional commits instead of the angular page.

@rbell517 What is the suggested way of doing this?

Can we update the commit message while trying to merge this branch? Or is there any better way of doing this?

@SSSantosh18 SSSantosh18 requested a review from rbell517 March 3, 2025 16:40
@rbell517
Copy link
Collaborator

rbell517 commented Mar 3, 2025

@rbell517 as changes in both Product and Spec client were related to projection and model fixes, they are breaking changes and we have combined them into a single PR. Let us know if there are any concerns with it.
Also, is there any specific process to do a major version bump? The angular commit documentation guideline related to schematic versioning in the contributing doc didn't mention anything about this.

@SSSantosh18 @shri2k2 To do a major version bump you add a trailing paragraph to your commit message starting with BREAKING CHANGE: that then describes the breaking change. It will cause the version bump to be major and add the breaking changes to the changelog. I will take an action to update the contributing docs to point to conventional commits instead of the angular page.

@rbell517 What is the suggested way of doing this?

Can we update the commit message while trying to merge this branch? Or is there any better way of doing this?

@shri2k2
Yes, you need to include it in the squash commit message, which you can set when you merge the change or enable auto-merge.

@Shri2109 Shri2109 requested a review from rbell517 March 4, 2025 14:05
@sam-rishi sam-rishi mentioned this pull request Mar 4, 2025
1 task
@rbell517 rbell517 enabled auto-merge (squash) March 4, 2025 22:39
@rbell517 rbell517 merged commit e9feff6 into ni:master Mar 4, 2025
6 checks passed
@Shri2109 Shri2109 deleted the users/shriram/fix-projection branch March 5, 2025 11:51
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.

5 participants