Move package API requests to one file, consolidate naming and internal API

[fiskus]

I need this refactoring because I want to be able to validate params for every package API request in one place

[codecov]

Codecov Report

Merging #2154 (e94fead) into master (836e812) will increase coverage by 0.02%.
The diff coverage is 35.38%.

@@            Coverage Diff             @@
##           master    #2154      +/-   ##
==========================================
+ Coverage   47.36%   47.38%   +0.02%     
==========================================
  Files         439      440       +1     
  Lines       21068    21095      +27     
  Branches     2432     2432              
==========================================
+ Hits         9978     9996      +18     
- Misses      10180    10189       +9     
  Partials      910      910              

Flag	Coverage Δ
api-python	89.87% <ø> (ø)
catalog	16.27% <35.38%> (+0.11%)	⬆️
lambda	93.44% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
catalog/app/containers/Bucket/PackageCopyDialog.js	0.00% <0.00%> (ø)
...talog/app/containers/Bucket/PackageCreateDialog.js	0.00% <0.00%> (ø)
...g/app/containers/Bucket/PackageDirectoryDialog.tsx	0.00% <0.00%> (ø)
...alog/app/containers/Bucket/PackageUpdateDialog.tsx	0.00% <0.00%> (ø)
catalog/app/containers/Bucket/requests/package.ts	38.88% <38.88%> (ø)
.../containers/Bucket/PackageDialog/PackageDialog.tsx	26.00% <100.00%> (-0.59%)	⬇️
catalog/app/containers/Bucket/requests/index.ts	100.00% <100.00%> (ø)

---

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 836e812...e94fead. Read the comment docs.

[nl0]

the general direction is right, but i think we should

1. adjust naming
2. remove unnecessary level of abstraction by calling req directly

[nl0]

i dont think this function should exist

1. its name doesnt reflect all its use cases, it looks like a leaked abstraction;
2. it doesnt have enough logic, i think it's better to just call req directly (you can parameterize body and response types to add type checking there)

[fiskus]

It's a place where I can validate the request body with Schema

[nl0]

you can validate it the same way by using type arg for req

[fiskus]

remove unnecessary level of abstraction by calling req directly

I can't do this, because my goal was to consolidate requests in one function where I can validate the request body

[nl0]

I can't do this, because my goal was to consolidate requests in one function where I can validate the request body

but requests are different, so are the bodies. you can type-check request body by simply using type arg when calling req directly.

[nl0]

ok, i see that by "validate" you probably mean some runtime validation, not type-checking, but i still dont quite get it

[fiskus]

function uploadManifest(req, s3, endpoint, workflow, body) {
  const schema = await s3.fetchFile(workflow.objectSchemaurl)
  const error = validateBody(schema)
  if (error.length) throw error
  return req(endpoint, body)
}

function useCreatePackage(body) {
  const req = useAPIConnector()
  const s3 = useS3()
  return uploadManifest(req, s3, "/packages", workflow, body)
}

function useCopyPackage(body) {
  const req = useAPIConnector()
  const s3 = useS3()
  return uploadManifest(req, s3,"/packages/promote",  workflow, body)
}

[fiskus]

I don't have working code for the next PR, but I wanted something like this ^

[nl0]

ok now i get it, but

1. uploadManifest doesnt seem like an appropriate name for this function
2. probably you dont need to type-check request body if you plan to validate it in runtime against a schema

[fiskus]

Do you have in mind what name could be appropriate?

[nl0]

Do you have in mind what name could be appropriate?

dunno, smth like requestWithValidation maybe, but it's your call since you have better idea of its potential use cases and the logic you want to add there

[fiskus]

I can be confident about an object's shape with type-checking, but Schema gives more possibilities like, for example, checking number values ({ min: 0, max: 256} for filesize).
Also, Schema can be different for different workflows

[nl0]

I can be confident about an object's shape with type-checking, but Schema gives more possibilities like, for example, checking number values ({ min: 0, max: 256} for filesize).
Also, Schema can be different for different workflows

true, but you must somehow guarantee that the schema is compatible with the type