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

Create new dataset (entity list) or property via the API #1105

Merged
merged 11 commits into from
Mar 18, 2024

Conversation

ktuite
Copy link
Member

@ktuite ktuite commented Mar 13, 2024

Backend part of these two issues:
getodk/central#575
getodk/central#576

Two new endpoints

POST /v1/projects/1/datasets
{ name: 'trees' }

POST /v1/projects/1/datasets/trees/properties
{ name: 'height' }

Still to do:

  • add dataset.create verb and use it
  • add documentation

What has been done to verify that this works as intended?

Tests.

Why is this the best possible solution? Were any other approaches considered?

I considered a different endpoint for adding properties. Maybe POSTing an array properties to a dataset, or PATCHing the dataset, but it didn't feel right. One reason is the existing PATCH .../dataset/:name endpoint is used for setting the approvalRequired flag, which feels like it's at a different level than dataset properties. Dataset properties have more to them, and they seem to deserve their own endpoint with POST .../properties.

We could have a matching GET .../properties, but for now, the properties are returned in the main dataset getter.

I tried to gate dataset creation via forms with the dataset.create verb, but it didn't quite make sense. We need to parse the whole form and get the dataset info out, and we would need to pass the auth mechanism along kind of deep inside form.createNew. Also, we have one Dataset.createOrMerge function that handles both new dataset and existing dataset cases, so we don't know if we're creating a dataset or not.

How does this change affect users? Describe intentional changes to behavior and behavior that could have accidentally been affected by code changes. In other words, what are the regression risks?

Does this change require updates to the API documentation? If so, please update docs/api.md as part of this PR.

Yes, I need to add documentation with this PR. (#1096)

Before submitting this PR, please make sure you have:

  • run make test and confirmed all checks still pass OR confirm CircleCI build passes
  • verified that any code from external sources are properly credited in comments or that everything is internally sourced

@matthew-white
Copy link
Member

add dataset.create verb and use it

Once dataset.create is in place, we had talked about the possibility of checking for it in form endpoints in some cases. That feels like a separate issue from getodk/central#575 and getodk/central#576, so I've filed getodk/central#612 about it. I think that change could come in a separate PR.

@ktuite ktuite marked this pull request as ready for review March 14, 2024 01:45
@ktuite ktuite requested a review from matthew-white March 14, 2024 01:45
@matthew-white
Copy link
Member

Ah I missed this in the PR description:

I tried to gate dataset creation via forms with the dataset.create verb, but it didn't quite make sense. We need to parse the whole form and get the dataset info out, and we would need to pass the auth mechanism along kind of deep inside form.createNew. Also, we have one Dataset.createOrMerge function that handles both new dataset and existing dataset cases, so we don't know if we're creating a dataset or not.

Could Dataset.createOrMerge() return information about what it did, either creating a dataset or not? If it did, then maybe Forms.createNew() could return that information to the endpoint. That way, the endpoint wouldn't need to pass auth to Forms.createNew(). Just a thought! But I think we could return to these questions later if we decide that we want to make changes as part of getodk/central#612.

lib/model/query/datasets.js Outdated Show resolved Hide resolved
lib/model/query/datasets.js Outdated Show resolved Hide resolved
lib/resources/datasets.js Outdated Show resolved Hide resolved
test/integration/api/datasets.js Outdated Show resolved Hide resolved
test/integration/api/datasets.js Show resolved Hide resolved
lib/resources/datasets.js Show resolved Hide resolved
lib/resources/datasets.js Outdated Show resolved Hide resolved
lib/resources/datasets.js Outdated Show resolved Hide resolved
test/integration/api/datasets.js Outdated Show resolved Hide resolved
lib/model/query/datasets.js Outdated Show resolved Hide resolved
test/integration/api/datasets.js Outdated Show resolved Hide resolved
@ktuite ktuite merged commit 85a0e3c into master Mar 18, 2024
5 checks passed
@ktuite ktuite deleted the ktuite/new_ds_prop_api branch March 18, 2024 22:53
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.

2 participants