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: Remove update_time and create_time from ServerInfo entity #1192

Closed
wants to merge 1 commit into from

Conversation

imjoey
Copy link
Member

@imjoey imjoey commented Jan 4, 2021

Please answer these questions before submitting a pull request

  • Why submit this pull request?

  • Bugfix

  • New feature provided

  • Improve performance

  • Related issues

Fixes #1176 .


Bugfix

  • Description

The empty values of create_time and update_time should not be returned from server_info interface provided by manager-api . Please see #1176 for details.

  • How to fix?

This PR is going to set create_time and update_time as omitempty to disable JSON serialization when returning back to frontend. In addition, adding the relevant test case for changes.

@juzhiyuan juzhiyuan requested review from nic-chen and liuxiran January 4, 2021 06:33
@codecov-io
Copy link

Codecov Report

Merging #1192 (b353041) into master (62d1c43) will decrease coverage by 0.05%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1192      +/-   ##
==========================================
- Coverage   42.38%   42.33%   -0.06%     
==========================================
  Files          31       31              
  Lines        1930     1930              
==========================================
- Hits          818      817       -1     
- Misses       1001     1002       +1     
  Partials      111      111              
Impacted Files Coverage Δ
api/internal/core/entity/entity.go 0.00% <ø> (ø)
api/internal/core/store/store.go 79.37% <0.00%> (-0.63%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 62d1c43...b353041. Read the comment docs.

ExpectCode int
ExpectMessage string
ExpectBody string
UnexpectedBody string
Copy link
Member Author

@imjoey imjoey Jan 4, 2021

Choose a reason for hiding this comment

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

To make it more clear, I just added a new field UnexpectedBody here, which reformatted the alignment.

@liuxiran
Copy link
Contributor

liuxiran commented Jan 4, 2021

I noticed that each field in the BaseInfo struct is not omited originly, but in this pr, we set create_time and update_time omitempty, so is it posible to set create_time and update_time out of BaseInfo struct?

@imjoey @nic-chen @starsz

@nic-chen
Copy link
Member

nic-chen commented Jan 4, 2021

I noticed that each field in the BaseInfo struct is not omited originly, but in this pr, we set create_time and update_time omitempty, so is it posible to set create_time and update_time out of BaseInfo struct?

@imjoey @nic-chen @starsz

Yes, a better solution is to separate create_time, update_time and ID.

In the next version, we will remove the consumer ID:
apache/apisix#2679

It will need create_time and update_time but no ID.

@imjoey imjoey force-pushed the set_update_create_omitempty branch from 9d021b2 to f525336 Compare January 4, 2021 14:32
Signed-off-by: imjoey <majunjiev@gmail.com>
@imjoey imjoey force-pushed the set_update_create_omitempty branch from f525336 to 5da99da Compare January 4, 2021 14:44
@imjoey imjoey changed the title fix: Omit update_time and create_time if empty for entities in manager-api fix: Remove update_time and create_time from ServerInfo entity Jan 4, 2021
@imjoey
Copy link
Member Author

imjoey commented Jan 4, 2021

@liuxiran @nic-chen Thanks for your advice. While unfortunately, for now ServerInfo still relies on the BaseInfo object for sorting in List interface. Since #1176 is not a critical bug, thus there seems no perfect solution right now, I prefer to close this PR and would reopen it once we work out an overall refactor design that fits for all entities.

@imjoey imjoey closed this Jan 4, 2021
@imjoey imjoey deleted the set_update_create_omitempty branch January 4, 2021 15:55
@imjoey imjoey restored the set_update_create_omitempty branch January 5, 2021 01:08
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.

bug: server info page shows create_time and update_time, which are not the content returned by the plugin
4 participants