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

Added http method filter to request handlers #25

Closed
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
4 participants
@coolya
Copy link
Contributor

coolya commented Feb 4, 2018

RequestHandler can now filter for specific HTTP methods that it is handling.
Add a migration to set a default filter that doesn't filter at all to preserve existing semantics.
Added test case for checking if the handler filters correctly.

Added http method filter to request handlers
RequestHandler can now filter for specific HTTP methods that it is handling.
Add a migration to set a default filder that doesn't filter at all to preserve existing semantics.
Added test case for checking if the handler filters correctly.
@qradimir

This comment has been minimized.

Copy link
Contributor

qradimir commented Feb 5, 2018

Hi, @coolya! Thank you for PR!

I've looked on it and got few questions:

  1. Do we need explicit any filtering node? Introducing this makes language model more complex and migration required. I think we can make validMethod child optional and generate any predicate if a child is absent.
  2. Do you expect something else except HttpMethod.SOMETHING expressions in SpecificMethodsFilter array? If not, it's better to avoid netty stuff in http-support DSL since it's undiscoverable and exposes dependency on netty lib. I think it's better to replace an expression with some concept that contains enum property with GET, POST and other options.
@bkolb

This comment has been minimized.

Copy link

bkolb commented Mar 14, 2018

Hi,

I also have a use case for this. Regarding your questions I think for 1) an explicit any is not required but would be nice. For 2) I think there should be a way to get to the low-level netty.
I currently have a use case where I have to register additional servlets (e.g. to do server sent event or websockets). In mbeddr I created an extension point for that as a temporary workaround but it would be nice if we could get rid of our httpsupport and just use yours.

@qradimir

This comment has been minimized.

Copy link
Contributor

qradimir commented Apr 19, 2018

Hi, @bkolb!

Regarding "any", we can just show in the editor the "any" keyword (that can be set as the empty text for the cell) having no "any" node under the hood. I think it covers your wishes, isn't it?

Talking about access to low-level netty, by "avoid netty stuff in http-support DSL" I didn't mean that we are going to get rid of all possibilities to access to the netty lib. Actually, I meant that a DSL user doesn't require to know the netty lib to work with http-support DSL. And this contradicts that a user editing SpecificMethodsFilter concept has to specify expressions of the HttpMethod type that is right from the netty lib. So the perfect scenario is when a user just writes GET, POST and it is generated to proper expressions. I hope you got my idea.

@bkolb

This comment has been minimized.

Copy link

bkolb commented Apr 20, 2018

OK. Sounds good.
Just to re-emphasize what we want to achieve:

  • get rid of the mbeddr http support. This means that we have to be able to write a migration script from our language to yours. In this sense yours should be a superset of ours (or at least on the same level)
  • Have an extension point (or other means) where I can register my own servlets, knowing that I have to deal with low level api of some sort.
@coolya

This comment has been minimized.

Copy link
Contributor Author

coolya commented Apr 20, 2018

Can we expose the low level netty api to enable advanced use cases and easier migration from the abstraction of the mbeddr http support dsl by allowing / downcast to lower semantics and then expose the underlying java class?

@qradimir

This comment has been minimized.

Copy link
Contributor

qradimir commented Apr 20, 2018

@bkolb IDEA platform already has an extension point where you can register your servlets. To do so you should implement org.jetbrains.ide.HttpRequestHandler class and register your implementation in com.intellij.httpRequestHandler EP. Also, you might be interested in implementing org.jetbrains.ide.RestService class. Does this extension point satisfy your wishes?

@coolya Are you going to introduce / operator on a request parameter on canHandle/handle queries? If so, it might be a good solution if it covers all your needs.

@ashatalin

This comment has been minimized.

Copy link
Member

ashatalin commented Sep 18, 2018

Hi, @coolya, @bkolb!

Can I ask you to reply yto @qradimir? This request is open for quite a long time and we should probably resolve it.

@bkolb

This comment has been minimized.

Copy link

bkolb commented Sep 19, 2018

Hi,
Sorry for the delay.
I think that should do for my use case.
Best,
Bernd

@ashatalin

This comment has been minimized.

Copy link
Member

ashatalin commented Mar 14, 2019

@qradimir what should we do with this pull-request?

@coolya

This comment has been minimized.

Copy link
Contributor Author

coolya commented Mar 15, 2019

I think we can abandon it. We will try to migrate the mbeddr http support to the current MPS http integration. If we should encounter particular blockers we will get back with specific requests.

@ashatalin ashatalin closed this Mar 22, 2019

@ashatalin

This comment has been minimized.

Copy link
Member

ashatalin commented Mar 22, 2019

Closing this pull request as "rejected" based on comment from @coolya

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.