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

fix get the openapi interface that contains namespace information for deleted items #4596

Merged
merged 6 commits into from
Oct 9, 2022

Conversation

CalebZYC
Copy link
Contributor

@CalebZYC CalebZYC commented Oct 3, 2022

What's the purpose of this PR

fix get the openapi interface that contains namespace information for deleted items.

Which issue(s) this PR fixes:

Fixes #4593

Brief changelog

fix get the openapi interface that contains namespace information for deleted items

Follow this checklist to help us incorporate your contribution quickly and easily:

  • Read the Contributing Guide before making this pull request.
  • Write a pull request description that is detailed enough to understand what the pull request does, how, and why.
  • Write necessary unit tests to verify the code.
  • Run mvn clean test to make sure this pull request doesn't break anything.
  • Update the CHANGES log.

@codecov
Copy link

codecov bot commented Oct 3, 2022

Codecov Report

Merging #4596 (b56e17c) into master (301d66d) will increase coverage by 0.16%.
The diff coverage is 75.00%.

@@             Coverage Diff              @@
##             master    #4596      +/-   ##
============================================
+ Coverage     46.32%   46.48%   +0.16%     
- Complexity     1559     1568       +9     
============================================
  Files           331      331              
  Lines         10356    10360       +4     
  Branches       1042     1043       +1     
============================================
+ Hits           4797     4816      +19     
+ Misses         5261     5246      -15     
  Partials        298      298              
Impacted Files Coverage Δ
.../server/service/ServerNamespaceOpenApiService.java 26.08% <0.00%> (ø)
...llo/portal/controller/ConfigsExportController.java 18.51% <0.00%> (ø)
...mework/apollo/portal/service/NamespaceService.java 76.53% <91.66%> (+4.13%) ⬆️
...rk/apollo/portal/service/ConfigsExportService.java 72.14% <100.00%> (ø)
...ervice/service/ReleaseMessageServiceWithCache.java 87.05% <0.00%> (+1.17%) ⬆️
...work/apollo/biz/message/DatabaseMessageSender.java 64.58% <0.00%> (+14.58%) ⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@nobodyiam
Copy link
Member

There are many places that consume the result of com.ctrip.framework.apollo.portal.service.NamespaceService#transformNamespace2BO, e.g. com.ctrip.framework.apollo.openapi.server.service.ServerNamespaceOpenApiService and com.ctrip.framework.apollo.portal.controller.ConfigsExportController.
However, only the controllers for the namespace UI need the deleted items, e.g. com.ctrip.framework.apollo.portal.controller.NamespaceController.

One way to fix the issue is to filter the deleted items in places that we don't need them, or we may add a flag and let the caller decide whether to include the deleted items or not, e.g. private NamespaceBO transformNamespace2BO(Env env, NamespaceDTO namespace, boolean includeDeletedItems).
I think the latter might be better. What do you think?

@CalebZYC
Copy link
Contributor Author

CalebZYC commented Oct 4, 2022

There are many places that consume the result of com.ctrip.framework.apollo.portal.service.NamespaceService#transformNamespace2BO, e.g. com.ctrip.framework.apollo.openapi.server.service.ServerNamespaceOpenApiService and com.ctrip.framework.apollo.portal.controller.ConfigsExportController. However, only the controllers for the namespace UI need the deleted items, e.g. com.ctrip.framework.apollo.portal.controller.NamespaceController.

One way to fix the issue is to filter the deleted items in places that we don't need them, or we may add a flag and let the caller decide whether to include the deleted items or not, e.g. private NamespaceBO transformNamespace2BO(Env env, NamespaceDTO namespace, boolean includeDeletedItems). I think the latter might be better. What do you think?

I agree with your opinion, but I found that the export namespace config file contains deleted items, this' right?

@nobodyiam
Copy link
Member

I agree with your opinion, but I found that the export namespace config file contains deleted items, this' right?

I think the export namespace config file should not contain deleted items, which means we also need to add the includeDeletedItems flag to com.ctrip.framework.apollo.portal.service.NamespaceService#findNamespaceBOs.

Copy link
Contributor

@mghio mghio left a comment

Choose a reason for hiding this comment

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

Please add some unit test in NamespaceServiceTest.

@nobodyiam
Copy link
Member

I think com.ctrip.framework.apollo.openapi.server.service.ServerNamespaceOpenApiService#getNamespaces should also exclude deleted items?

Copy link
Member

@nobodyiam nobodyiam left a comment

Choose a reason for hiding this comment

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

LGTM

@nobodyiam nobodyiam merged commit 25fa47f into apolloconfig:master Oct 9, 2022
@github-actions github-actions bot locked and limited conversation to collaborators Oct 9, 2022
@CalebZYC CalebZYC deleted the issue-4593 branch November 27, 2022 16:12
@nobodyiam nobodyiam added this to the 2.1.0 milestone Feb 5, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

openapi获取某个Namespace信息接口中包含删除的item
3 participants