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

Batch resolution phase #302

Merged
merged 3 commits into from
Apr 10, 2017
Merged

Batch resolution phase #302

merged 3 commits into from
Apr 10, 2017

Conversation

benwilson512
Copy link
Contributor

No description provided.

@benwilson512 benwilson512 changed the base branch from master to v1.3 March 28, 2017 13:41
@benwilson512
Copy link
Contributor Author

@skosch I'm moving the batch resolution ability to Absinthe itself as none of its concerns are really Plug specific, and it seems I'm likely to need them in Absinthe itself anyway.

As I'm doing this though it seems clear that would I would REALLY like is essentially

Absinthe.Pipeline.Batch.run(prep_phases, resolution_phases, result_phases, blueprints)

This became particularly clear when trying to write a test for just the resolution logic I had so far. There was easy way to get a set of blueprints that were up to the correct point.

The overall logic here is non trivial because errors in the prep phases needs to require jumping over the resolution phases for that doc and moving to the result phases. Fortunately we've already done 98% of this in the Absinthe.Plug PR, but I'm wondering if it really belongs here.

@bruce I think we could make the API even simpler to just:

Absinthe.Pipeline.Batch.run(phases, blueprints)

With the introduction of pipeline tags that we've talked about. @skosch pipeline tags are an idea where in a pipeline you could have:

[
  {:section, :prep}
  SomePhase,
  SomeOtherPhase,
  {:section, :resolution}
  ResolutionPhase
  {:section, :result}
  ResultPhase
]

Thoughts?


{:ok, %{bp_root | resolution: %{resolution | acc: acc}}}
end
end
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@bruce note I've decided to go back on my Before/After resolution phase stuff. It kept producing a lot boilerplate everywhere. Instead i've simply got an option you can pass to the Resolution phase that turns on / off running the callbacks.

@skosch
Copy link
Contributor

skosch commented Mar 28, 2017

@benwilson512 thank you for looking into this!

I obviously don't know the Absinthe codebase well enough to comment on how to minimize the pain of passing phases and results around. Do you see an advantage in the pipeline tags idea over, say, having an alternate run definition?

def run(%{prep, resolution, result}, blueprints) do ...
def run(phases, blueprints) do ...

@tlvenn
Copy link
Member

tlvenn commented Apr 3, 2017

Any chance to merge this soon if it's ready to get us started with transport batching ?

@benwilson512
Copy link
Contributor Author

So in the end most of this is going to go inside of Absinthe.Plug itself. There's a number of things that are a bit odd about handling document batches in Absinthe at the moment, so I think for 1.3 it all goes in Absinthe.Plug and we extract it to Absinthe itself for 1.4 when we look at subscriptions.

@benwilson512 benwilson512 merged commit a990580 into v1.3 Apr 10, 2017
@benwilson512 benwilson512 deleted the batch-resolution-phase branch April 10, 2017 20:10
benwilson512 added a commit that referenced this pull request Apr 13, 2017
* Add logging, with support for filtering (#280)

* Support log: false configuration

* Address some feedback, clean up defaults (#282)

* Middleware (#254)

* minor optimization around arguments

* basic functionality

* almost green tests

* green tests

* handle middleware callbacks

* move plugins over to the middleware namespace

* forgot files as usual

* significant progress

* remove async and batch indirection

* rename Absinthe.Resolution.Middleware => Absinthe.Middleware

* handle before and after resolution

* some middleware testing

* improved resolution state names

* plug is now middleware

* finalize name for ensuring middleware

* doc updates

* it helps if you commit all the files

* finalizing names

* handle commentary

* better doc header

* Schema ensure query type (#226)

* Add query type rule

* Start to fix tests with querytype

* Add test

* fix more tests with query type

* Remove double imports

* Remove unused Type

* Add Explanation to FieldImportExist

* Fix the last two tests

Rewinding my own mistakes

* Fixed valid_schema

* ensure anon function form works

* context modification test

* more docs

* updated travis

* last doc tweak

* changelog update

* version bump

* correct version

* typo fix

* Date and Time Scalars (#256)

* added utc_datetime as a built-in scalar

* added naive datetime as a built-in scalar

* added date as a built-in scalar

* added time as a built-in scalar

* removed datetime types from built_ins

* moved datetime types to new extensions module

* added stricter pattern matching to datetime parser

* elixir 1.4 for travis

* fixed parsing and renamed module

* bumped elixir version

* bumped elixir version for travis

* testing against UTC offsets

* testing parsing 0-utc offset

* parsers are passed a blueprint input struct instead of a string

* added high-level tests for custom types

* build fix

* Use Markdown list syntax for nicer rendering (#285)

* changelog entry

* doc fix

* Support for `to_string/1` compatible error tuples in resolution  (#286)

* Use to_string for error messages

Sometimes you get other types in {:error, value}, like atom and integers,
and instead of people handling the converation themselves it's better just
returning the string equivalent of the atom inline with what json libraries
like Poison do or any other type that can be a string.

An example would be if you are doing `with` in you body you get the error
message from the library you are using. In guardian case you could get:
`{:error, :token_expired}`

* Changes for bruce

* Add changelog entry

* Moving TODOs to PR

* better changelog for beta

* fix bug where root query and mutation types had nil identifiers

* version bump

* provide test helping primer function

* docs

* Tuning (#292)

* do expansion prior so that we don't have to do it in resolution

* faster field applies check

* get rid of infinite loops and improved type condition handling

* add renamed file, and code reorganization

* so close to green

* tests are green at last

* remove bug line

* Fix #244

* Interface subtype (#293)

* Add failing test for interface subtype (#288)

This shows that an implementor of an interface with a more specific
field than expected will not compile.

* test fixes

* fixes to enable absinthe relay

* test prime bug fix

* more assertive

* resurrect plugin module as the before/after callbacks from middleware

* middleware.t => middleware.spec

* move scalar and enum serialization to the result phase (#296)

* remove commented out debug statements

* Add Absinthe.Pipeline.replace/3 (#300)

* Add Pipeline.replace/3

* Better type spec for Type.Directive

* Make Pipeline.replace/3 more flexible

* Clean up some warnings

* Prepare 1.3.0-beta.2

* Fix doc typo

* Another doc fix

* laxer test time for travis

* Batch resolution phase (#302)

* basic work on batching resolution phase

* bug fix

* remove warnings

* Changed HandleAuth to HandleError (#297)

* minor tweaks to support absinthe plug batching

* Extensions (#306)

* make the whole pipeline work with blueprints

* extensions test

* bug fixes

* version bump

* if a validation phase cannot jump, it returns an error instead of an ok tuple from the phase

* remove strange indentation

* fix performance issue, need to fix some tests though (#307)

* fix performance issue, need to fix some tests though

* green tests
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.

None yet

3 participants