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

AJ's work on for data library #1

Merged
merged 207 commits into from
Dec 4, 2024
Merged

AJ's work on for data library #1

merged 207 commits into from
Dec 4, 2024

Conversation

AlbertSnows
Copy link
Contributor

This is just hosting my changes while I work on adding endpoints, it is not in a ready state and should not be merged without caution.
Link to JIRA ticket if there is one:

New Features

Breaking Changes

Bug Fixes

Improvements

Dependency updates

Deployment changes

Copy link

github-actions bot commented Sep 9, 2024

The style in this PR agrees with black. ✔️

This formatting comment was generated automatically by a script in uc-cdis/wool.

author Albert Snow <[email protected]> 1724698038 -0500
committer Albert Snow <[email protected]> 1725904980 -0500

# This is a combination of 20 commits.tree 4536d9e23c632e2d1db5b6390967d3dc9ac1c129
parent 0aeb863
author Albert Snow <[email protected]> 1724698038 -0500
committer Albert Snow <[email protected]> 1725904756 -0500

# This is a combination of 19 commits.
# This is the 1st commit message:

add sh

# This is the commit message #2:

poetry deps

# This is the commit message #3:

more sh moving

# This is the commit message #4:

switchin branches

# This is the commit message #5:

remove

# This is the commit message #6:

adding read all lists
readme change

# This is the commit message #7:

add delete

# This is the commit message #8:

fix delete

# This is the commit message #9:

remove duplicate

# This is the commit message #10:

param docs

# This is the commit message #11:

setting up by id endpoints

# This is the commit message #12:

setting up dal

# This is the commit message #13:

implementing update list, delete all lists, get list, and read list

# This is the commit message #14:

formatting

# This is the commit message #15:

get list by id works

# This is the commit message #16:

delete by id works

# This is the commit message #17:

adding upsert list by id

# This is the commit message #18:

upsert insert works

# This is the commit message #19:

upsert update works

# This is the commit message #20:

add sh

poetry deps

more sh moving

switchin branches

remove

param docs

setting up by id endpoints

setting up dal

implementing update list, delete all lists, get list, and read list

get list by id works

delete by id works

adding upsert list by id

upsert update works
add route docs
change test
update lock for security issue
fix the id put and added patch
decouple behavior
update put lists endpoint to handle various forms of data that need to be updated or created
updating dal functions to handle replacing and crating lists better
fix upsert
fixing tests
update lines to use endpoint lambdas
more work to fix tests
fixing dal to use list id
fix replacer
fix user_id param?
and default auth
updating routes to use name/creator combo
minor name change
adding a test
add authz to blacklist
finalizing tests for /lists endpoint
create by id test file
updating routes
moving data out of lists
adding basic first test for id
@Avantol13
Copy link
Collaborator

Required before merge

Manually testing API, ran into an issue. When I PUT /lists/{{list_id}} with modifications, the newly updated list has a different id, meaning I cannot READ /lists/{{list_id}} anymore.

The expected behavior is that updating the content of a list using the list_id should update in place and not create a new list_id.

@AlbertSnows
Copy link
Contributor Author

Required before merge

Manually testing API, ran into an issue. When I PUT /lists/{{list_id}} with modifications, the newly updated list has a different id, meaning I cannot READ /lists/{{list_id}} anymore.

The expected behavior is that updating the content of a list using the list_id should update in place and not create a new list_id.

This is expected behavior currently. The put by id endpoint replaces the list instead of updating it. As such, the id used to hit the endpoint is no longer valid since that id is deleted. Instead, it's expected that the user use the new id provided in the response after the replacement if they want to do another put request.
This is corroborated in the docs via this line:

this fully replaces the list with what's provided

and also from some previous comments in the PR.
However if we're not happy with this we can change it, we may just need to clarify intent/behavior in the docs

@k-burt-uch
Copy link
Collaborator

Required before merge

Manually testing API, ran into an issue. When I PUT /lists/{{list_id}} with modifications, the newly updated list has a different id, meaning I cannot READ /lists/{{list_id}} anymore.
The expected behavior is that updating the content of a list using the list_id should update in place and not create a new list_id.

This is expected behavior currently. The put by id endpoint replaces the list instead of updating it. As such, the id used to hit the endpoint is no longer valid since that id is deleted. Instead, it's expected that the user use the new id provided in the response after the replacement if they want to do another put request. This is corroborated in the docs via this line:

this fully replaces the list with what's provided

and also from some previous comments in the PR. However if we're not happy with this we can change it, we may just need to clarify intent/behavior in the docs

We should definitely change this. If a user updates a list, the list ID should remain the same. This will create issues when a user attempts to make multiple updates to a list. Each update will require a new ID. If we want to extend and expand this feature to allow users to share their lists (which we do), then updating something like the name will cause all references to the list to break.

@AlbertSnows AlbertSnows merged commit f2db88f into feat/origin Dec 4, 2024
4 checks passed
@AlbertSnows AlbertSnows changed the title [DNM] AJ's work on for data library AJ's work on for data library Dec 4, 2024
@AlbertSnows AlbertSnows mentioned this pull request Dec 5, 2024
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.

3 participants