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

feat: return 200 (not 204) for PUT requests to entity verticles #326

Merged
merged 1 commit into from
Aug 25, 2023

Conversation

dapeng-wang
Copy link

No description provided.

@dapeng-wang dapeng-wang requested a review from s4heid June 2, 2023 12:30
@dapeng-wang dapeng-wang requested a review from a team as a code owner June 2, 2023 12:30
@sonarcloud
Copy link

sonarcloud bot commented Jun 2, 2023

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

@pk-work
Copy link
Contributor

pk-work commented Jun 12, 2023

In the last NeonBee Committer Meeting we discussed this PR with @kristian . We came to the following conclusion:

-> Responds with a Location Header / Hint, use Location as returned by EntityVerticle
-> Responds with a Entity that defines the primary key completely then generate relative location header in ODataEndpoint, e.g. EntitySetName(PrimaryKey) (check if helper method in Olingo is available)
-> Responds without a entity (e.g. empty future) or the returned entity does not contain the full primary key, continue as of now and NOT set the location header
-> In future, to be able to use absolute URLs in Location header instead: provide helper method in NeonBee to generate absolute header based on currently RoutingContext and also add new „absoluteURLPrefix“ configuration (e.g. /backend as this is added by AppRouter not NeonBee)

s4heid
s4heid previously approved these changes Aug 24, 2023
Copy link
Contributor

@s4heid s4heid left a comment

Choose a reason for hiding this comment

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

Only found the two typos in the doc. Implementation looks good to me :)

I created #374 to tackle the leftovers of Pascals comment. As agreed in the last committer meeting, we split the implementation.

docs/usage/verticles/EntityVerticle.md Outdated Show resolved Hide resolved
docs/usage/verticles/EntityVerticle.md Outdated Show resolved Hide resolved
@sonarcloud
Copy link

sonarcloud bot commented Aug 25, 2023

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

@dapeng-wang dapeng-wang merged commit 4bdf45a into main Aug 25, 2023
10 checks passed
@dapeng-wang dapeng-wang deleted the feat/update_entity branch August 25, 2023 08:33
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.

None yet

3 participants