Skip to content

fix: query string is incorrectly serialized for primitive parameters (#103) #109

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 3 commits into from
Apr 17, 2020
Merged

fix: query string is incorrectly serialized for primitive parameters (#103) #109

merged 3 commits into from
Apr 17, 2020

Conversation

kokovtsev
Copy link
Contributor

No description provided.

@kokovtsev kokovtsev marked this pull request as draft April 12, 2020 12:49
@kokovtsev kokovtsev marked this pull request as ready for review April 12, 2020 12:49
@kokovtsev kokovtsev force-pushed the bugfix-primitive-query-parameters branch from e38690d to cbcd3bc Compare April 15, 2020 07:46
raveclassic
raveclassic suggested changes Apr 15, 2020
View reviewed changes
src/language/typescript/2.0/serializers/operation-object.ts
@@ -411,8 +411,8 @@ export const serializeQueryParameterObject = (
case 'number':
case 'boolean': {
const f = serializedFragment(
`value => encodeURIComponent('${parameter.name}') + '=' + encodeURIComponent(value)`,
[],
`value => some(encodeURIComponent('${parameter.name}') + '=' + encodeURIComponent(value))`,
Copy link
Contributor

Choose a reason for hiding this comment

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

I see that you add some in all switch branches, would it make more sense to keep this function simpler and operate on simple types and wrap its result in some on the call site?

src/language/typescript/3.0/serializers/parameter-object.ts Outdated
[serializedDependency('serializePrimitiveParameter', pathToUtils)],
`value => option.fromEither(serializePrimitiveParameter('${style}', '${parameter.name}', value))`,
[
serializedDependency('option', 'fp-ts'),
Copy link
Contributor

Choose a reason for hiding this comment

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

although it's ok to import modules from fp-ts index in the end project, here in the lib we'd like to minimise imports, so could you import fromEither directly from fp-ts/lib/Option here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Aren't we going to have collisions if some other serializedFragment imports fromEither from another module?

Copy link
Contributor

Choose a reason for hiding this comment

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

The same is for option/either etc from fp-ts. We don't have general solution at the moment.

src/language/typescript/3.0/serializers/parameter-object.ts Outdated
[],
),
);
}
if (ArraySchemaObjectCodec.is(schema)) {
return right(
serializedFragment(
`value => serializeArrayParameter('${style}', '${parameter.name}', value, ${explode})`,
[serializedDependency('serializeArrayParameter', pathToUtils)],
`value => option.fromEither(serializeArrayParameter('${style}', '${parameter.name}', value, ${explode}))`,
Copy link
Contributor

Choose a reason for hiding this comment

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

idem

src/language/typescript/common/data/serialized-fragment.ts
)`,
[serializedDependency('option', 'fp-ts'), serializedDependency('pipe', 'fp-ts/lib/pipeable')],
[],
)
: serializedFragment(`some((${fn})(${a}))`, [serializedDependency('some', 'fp-ts/lib/Option')], []),
: serializedFragment(
`pipe(
Copy link
Contributor

Choose a reason for hiding this comment

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

minor: is it really necessary to change direct function invocation to pipe?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's just a matter of readability; direct invocation turns out less readable even after applying prettier. Shall I switch it back to the direct call?
image

Copy link
Contributor

Choose a reason for hiding this comment

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

let's skip

@kokovtsev kokovtsev requested a review from raveclassic April 17, 2020 07:14
@raveclassic raveclassic merged commit 94a449f into devexperts:master Apr 17, 2020
raveclassic pushed a commit that referenced this pull request Apr 17, 2020
@kokovtsev @raveclassic
fix: query string is incorrectly serialized for primitive parameters (#…
be39d47 
…103) (#109)
Nedgeva added a commit to Nedgeva/swagger-codegen-ts that referenced this pull request Jul 29, 2020
@Nedgeva
fix: remove timezone detection from DateFromISODateStringIO
02c58a6 
fix: support windows os environment

fixed multiple errors that prevents from successful run on windows machine

feat: sketch file format

closes devexperts#76

refactor: relax specLikeCodec codec for sketch-121 schema

lint: remove unnecessary dependency

refactor: rename sketch parser to contain format version

refactor: fix opacity rendering

refactor: fix numeric rendering

chore: split document.ts to different files

chore: generate assets

docs: add a comment about a sketch-like structure

2.0.0-alpha.6

feat: implement support of kebab case in property names (devexperts#83)

closes devexperts#82

refactor: use Integer type from io-ts (devexperts#84)

BREAKING CHANGE: removed empty utils file

2.0.0-alpha.7

fix: fix encodeURIComponent in query serialization

2.0.0-alpha.8

fix: fix encodeURIComponent in query serialization

2.0.0-alpha.9

fix: fix '..' and '.' in spec names

BREAKING CHANGE: generate doesn't include `out` path in spec output
BREAKING CHANGE: language doesn't accept `out` path and can return `Fragment` to write to `out` directly

refactor: simplify resolveRef

BREAKING CHANGE: language is now a Reader of ResolveRefContext
BREAKING CHANGE: resolveRef now requires decoder as a second argument

chore: fix build

build: update prettier and eslint

style: update prettier and eslint

2.0.0-alpha.10

fix: revert join to resolve

feat: support cwd as `generate` argument

2.0.0-alpha.11

build: cleanup publishing artifact

build: cleanup publishing artifact

fix(sketch): avoid collision for names with counter

feat: support extract layers from sketch file

refactor: use traverseOptionEither

fix(sketch): add prefix to original layer name

2.0.0-alpha.12

fix: fix ResponseValidationError class in client

Fixes issue devexperts#93

fix: fix ResponseValidationError class in client

fix: fix ResponseValidationError class in client

refactor

fix: fix ResponseValidationError class to properly extend Error (devexperts#94)

closes devexperts#93

2.0.0-alpha.13

chore: update @devexeperts/* dependencies

2.0.0-alpha.14

feat: add `nullable` support for OpenAPI3

fix: fix OperationObject name generation without operationId

BREAKING CHANGE: if operation id is missing then url/pattern is included in the name

2.0.0-alpha.15

feat: expand sketch schema for support foreign symbols

feat: support date-time format (devexperts#100)

Added io-ts codec that supports date ignoring time

2.0.0-alpha.16

fix: add year padding to the codec (devexperts#101)

Added year padding to the codec to cover the case when backend returns  that should be treated as valid date

2.0.0-alpha.17

feat: filter operations response

BREAKING CHANGE: non-successful response types are filtered out of resulting response type

fix: incorrect generation "deprecated" jsdoc for method (devexperts#111)

fix: void response missing from union (devexperts#104) (devexperts#110)

BREAKING CHANGE: void response is added to resulting response type

fix: query string is incorrectly serialized for primitive parameters (devexperts#103) (devexperts#109)

build(deps): bump acorn from 5.7.3 to 5.7.4

Bumps [acorn](https://github.com/acornjs/acorn) from 5.7.3 to 5.7.4.
- [Release notes](https://github.com/acornjs/acorn/releases)
- [Commits](acornjs/acorn@5.7.3...5.7.4)

Signed-off-by: dependabot[bot] <[email protected]>

2.0.0-alpha.18

feat: support arbitrary strings as type names in TS for OA3

closes devexperts#114

BREAKING CHANGE: type names and IO constant names now replace all non-alphanumeric characters with '_'

2.0.0-alpha.19

feat: support arbitrary names in path/query params

BREAKING CHANGE: query/path param names are now escaped

2.0.0-alpha.20

fix: change function name in utils file

2.0.0-alpha.21

fix(ts-utils): remove timezone detection from DateFromISODateStringIO, add tests

fix(ts-utils): refactor DateFromISODateStringIO

fix(ts-utils): rename vars in util

fix(ts-utils): remove tests

fix(ts-utils): remove export keyword
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@raveclassic raveclassic raveclassic approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@kokovtsev @raveclassic