I would prefer to add this test to the existing SuggestBuilderTests and not create a new test class for this. If I understand the existing test in CompletionSuggestSearchIT correctly this is only testing pasting, so it should work, or am I missung something?

@areek @cbuescher One thing I request you both to comment on in particular is adding the NamedWriteable parameter to the registerSuggester method. I don't know if this will cause any issues with registering custom suggester plugins, but adding this parameter is required so that we can register the associated builder with the custom suggester in the namedWriteableRegistry.

There is a getBuilderPrototype() method each suggester needs to provide which should return the appropriate builder prototype for each suggester. You could register the Suggester itself and then get its class and its builder prototype.

This is a great idea, I made the change, thanks @cbuescher !

I think registering the smoothing models should also go somewhere into SearchModule. Since they are only used in PhraseSuggestionBuilder, I would probably put them alongside the build-in suggestion builders in configureSuggesters()

Done

Nit: you could move the stuff from provideNamedWritableRegistry() here and then in provideNamedWritableRegistry() return the internal NamedWritableRegistry. Maybe less error prone when maintaining the test later to only have one registry around.

Done

Maybe just call it suggest?

Done

@cbuescher @areek I added this fromStart method parameter to differentiate, when parsing from x-content, between the situation where the suggest is part of a larger search request, and when the suggest is on its own as part of the suggest API. This may go away later when we merge the implementation of the search with suggest and standalone suggest into one, as @areek suggested before, but what I noticed was in some places, the parsing was failing and always returning back "suggestion name not found", even though there may have been other issues with the parsing. Also, valid JSON was not being parsed correctly by the suggest API. The reason is because in the search with suggest scenario, it has already parsed the starting token for the suggest object, whereas in the suggest API scenario, the starting token has not yet been parsed. In some places, we were adding another parser.nextToken() to account for this, but it wasn't being applied consistently and it was error prone to manage that. I feel having the method itself have a parameter that declares the intentions of the parsing (whether we have already parsed the starting { or not) is more clear and less error prone, but I welcome your thoughts.

I see the problem but I find adding this extra "control" parameter confusing. I would either make sure that the parser is always on the start token on the caller side (I think in SearchSourceBuilder this is always the case, in tests and in RestSuggestAction we should make sure it is) and otherwise throw an error, or check if parser.currentToken == null inside the SuggestBuilder parse method and advance the token pointer if so. This btw. is what ObjectParser#parse() does by default. I think this would be the best solution.

+1 to this solution