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

External import supports binary base type #580

Closed
wants to merge 1 commit into from
Closed

Conversation

ferozco
Copy link
Contributor

@ferozco ferozco commented Mar 27, 2020

Closes #579

Before this PR

After this PR

==COMMIT_MSG==
External imports can have binary as their base type
==COMMIT_MSG==

Possible downsides?

@changelog-app
Copy link

changelog-app bot commented Mar 27, 2020

Generate changelog in changelog/@unreleased

Type

  • Feature
  • Improvement
  • Fix
  • Break
  • Deprecation
  • Manual task
  • Migration

Description

External imports can have binary as their base type

Check the box to generate changelog(s)

  • Generate changelog entry

@policy-bot policy-bot bot requested a review from dansanduleac March 27, 2020 21:12
@carterkozak
Copy link
Contributor

This would be easy to misuse. We have a ton of special handling for binary streaming around dealiasing that may or may not work as expected with external type imports.

@iamdanfox
Copy link
Contributor

iamdanfox commented Mar 30, 2020

Ok so in order to enable @mglazer to write:

types:
  imports:
    Response:
      base-type: binary
      external:
        java: javax.ws.rs.core.Response

I think we'd need to do some special-casing in the dialogue generator to just ignore external imports with base-type: binary and just code-generate InputStream instead... this feels a bit odd because everywhere else we've respected people's external imports.

@carterkozak
Copy link
Contributor

It breaks conjure-undertow as well

@iamdanfox
Copy link
Contributor

I think this is just what happens when you rely on things that are not inside the conjure ecosystem unfortunately :/. @mglazer is there any feasibility to use just a binary response type instead of the jaxrs one?

@mglazer
Copy link
Contributor

mglazer commented Mar 30, 2020

So the reason why I switched to javax.ws.rs.Response in the first place is that I needed to set a Content-Disposition header so this particularly endpoint would be treated as an attachment endpoint. I agree this shouldn't have been made into a conjure endpoint in the first place if it's just serving an arbitrary file, but this is where we are.

What I've done for now that seems to be the only thing that works is setting jerseyBinaryAsResponse as a conjure option, which seems to work pretty well, but I suspect if we ever stop using the Jersey endpoints, something will break?

@stale
Copy link

stale bot commented Apr 13, 2020

This PR has been automatically marked as stale because it has not been touched in the last 14 days. If you'd like to keep it open, please leave a comment or add the 'long-lived' label, otherwise it'll be closed in 7 days.

@stale stale bot added the stale label Apr 13, 2020
@stale stale bot closed this Apr 23, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Can't use binary as a base type
4 participants