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

solr v2 api manager #839

Closed
wants to merge 5 commits into from
Closed

Conversation

wickedOne
Copy link
Collaborator

@wickedOne wickedOne commented Jul 10, 2020

@mkalkbrenner a quick draft just to see whether you like where this is going.

keep in mind that this has been tested on solr non-cloud mode for the schema & config api only, so it's likely to need some additional work.

a little background on some decisions

  • the manager is final because of the poll @ Use of 'final' and 'private' in Psr18Adapter #795
  • the persist and flush is used because of familiarity. and while flush in reality is a reload i personally see no problem with that.
  • i think solarium should only provide a ApiV2ResponseNormalizerInterface rather than actual normalizers (see below)
  • the endpoint is optional in the constructor and a wither is provided to set it seperately as, for example, one might want to update multiple schema's in one iteration and re-instantiating a manager for that seems unnecessary.
  • i'm not sure whether solarium should provide serializable models out of the box (see below)

normalizers

writing your own normalizers should be easy and by not providing them out of the box, it keeps us from adding dependencies we don't really need. for instance a normalizer could look like:

<?php

declare(strict_types=1);

namespace App\Manager;

use App\Model\MySchemaResponseModel;
use JMS\Serializer\SerializerInterface;
use Solarium\Core\Query\Result\ResultInterface;
use Solarium\Manager\Contract\ApiV2ResponseNormalizerInterface;

class SolrSchemaResponseNormalizer implements ApiV2ResponseNormalizerInterface
{
    /**
     * @var JMS\Serializer\SerializerInterface
     */
    private $serializer;

    /**
     * @param \JMS\Serializer\SerializerInterface $serializer
     */
    public function __construct(SerializerInterface $serializer)
    {
        $this->serializer = $serializer;
    }

    /**
     * {@inheritdoc}
     */
    public function normalize(ResultInterface $result)
    {
        return $this->serializer->deserialize($result->getBody(), MySchemaResponseModel::class, 'json');
    }
}

then creating the manager should be as simple as

<?php

declare(strict_types=1);

namespace App;

use App\Manager\SolrSchemaResponseNormalizer;
use Solarium\Client;
use Solarium\Manager\Config\SchemaApiConfig;

class SolrSchemaService
{
    /**
     * @var \Solarium\Manager\ApiV2Manager
     */
    private $manager;

    /**
     * @param \Solarium\Client                          $client
     * @param \App\Manager\SolrSchemaResponseNormalizer $normalizer
     */
    public function __construct(Client $client, SolrSchemaResponseNormalizer $nomalizer)
    {
        $this->manager = new ApiV2Manager($client, new SchemaApiConfig(), $nomalizer);
    }
}

models

i'm not sure whether we should provide serializable models as they can get pretty complicated for specific use cases and perhaps we're just better off if we leave them out. on the other hand, we can provide several basic models and have those who require more complex types create them by their own.

i've included one model for demonstration purpose. imagine when you want to set a config property it would be something like:

<?php

declare(strict_types=1);

namespace App;

use App\Manager\SolrSchemaResponseNormalizer;
use Solarium\Client;
use Solarium\Manager\Config\SchemaApiConfig;
use Solarium\Manager\Model\Property;

class SolrConfigService
{
    /**
     * @var \Solarium\Manager\ApiV2Manager
     */
    private $manager;

    /**
     * @param \Solarium\Client $client
     */
    public function __construct(Client $client)
    {
        $this->manager = new ApiV2Manager($client, new ConfigApiConfig());
    }

    public function setProperty(string $name, string $value): void
    {
        $this->manager
            ->addCommand(ConfigApiConfig::SET_PROPERTY, new Property($name, $value))
            ->persist()
            ->flush()
        ;
}

fixes #838

@codecov-commenter
Copy link

codecov-commenter commented Jul 10, 2020

Codecov Report

Merging #839 into master will decrease coverage by 1.10%.
The diff coverage is 0.00%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master     #839      +/-   ##
============================================
- Coverage     91.13%   90.02%   -1.11%     
- Complexity     3817     3853      +36     
============================================
  Files           330      336       +6     
  Lines          8201     8302     +101     
============================================
  Hits           7474     7474              
- Misses          727      828     +101     
Flag Coverage Δ Complexity Δ
#unittests 90.02% <0.00%> (-1.11%) 3853.00 <36.00> (+36.00) ⬇️
Impacted Files Coverage Δ Complexity Δ
src/Manager/ApiV2Manager.php 0.00% <0.00%> (ø) 9.00 <9.00> (?)
src/Manager/Command/CommandCollection.php 0.00% <0.00%> (ø) 16.00 <16.00> (?)
src/Manager/Config/ConfigApiConfig.php 0.00% <0.00%> (ø) 3.00 <3.00> (?)
src/Manager/Config/SchemaApiConfig.php 0.00% <0.00%> (ø) 3.00 <3.00> (?)
src/Manager/Model/Property.php 0.00% <0.00%> (ø) 4.00 <4.00> (?)
src/Manager/Normalizer/NoopResponseNormalizer.php 0.00% <0.00%> (ø) 1.00 <1.00> (?)
... and 2 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update fe70c08...c082540. Read the comment docs.

@wickedOne
Copy link
Collaborator Author

wickedOne commented Jul 10, 2020

perhaps we could provide a generic normalizer for those who don't care about the response?
1ac93ca

@wickedOne wickedOne force-pushed the api-manager branch 4 times, most recently from 6d5ca2f to c082540 Compare July 10, 2020 12:45
@wickedOne
Copy link
Collaborator Author

also, @dmaicher @thomascorthals @thePanz feel free to comment when you have any input on this..

@thomascorthals
Copy link
Member

The generic normalizer is a great idea.

If flush is actually a reload, maybe we should call it that for correctness's sake? We can still have flush as an alias for reload. Or perhaps the other way around if that would make more sense. As long as someone thinking "I need to reload" can do just that and have it work as expected.

@wickedOne
Copy link
Collaborator Author

i initially chose flush because of familiarty for those using an orm like doctrine and because it's not clear to everyone that you need to reload the core in order to activate your changes.

call it reload and hava a flush method as alias indeed sounds like the best of both worlds.

renamed flush to reload and added a flush wrapper instead
@codecov-io
Copy link

Codecov Report

Merging #839 (6bdb382) into master (7cf99c9) will decrease coverage by 0.91%.
The diff coverage is 0.00%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master     #839      +/-   ##
============================================
- Coverage     91.21%   90.29%   -0.92%     
+ Complexity     3802     3323     -479     
============================================
  Files           328      334       +6     
  Lines          8146     8212      +66     
============================================
- Hits           7430     7415      -15     
- Misses          716      797      +81     
Flag Coverage Δ Complexity Δ
unittests 90.29% <0.00%> (-0.92%) 0.00 <36.00> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ Complexity Δ
src/Manager/ApiV2Manager.php 0.00% <0.00%> (ø) 10.00 <10.00> (?)
src/Manager/Command/CommandCollection.php 0.00% <0.00%> (ø) 15.00 <15.00> (?)
src/Manager/Config/ConfigApiConfig.php 0.00% <0.00%> (ø) 3.00 <3.00> (?)
src/Manager/Config/SchemaApiConfig.php 0.00% <0.00%> (ø) 3.00 <3.00> (?)
src/Manager/Model/Property.php 0.00% <0.00%> (ø) 4.00 <4.00> (?)
src/Manager/Normalizer/NoopResponseNormalizer.php 0.00% <0.00%> (ø) 1.00 <1.00> (?)
src/Component/ResponseParser/MoreLikeThis.php 88.23% <0.00%> (-0.66%) 9.00% <0.00%> (-2.00%)
src/Core/Client/Adapter/Curl.php 53.75% <0.00%> (-0.58%) 31.00% <0.00%> (ø%)
src/Core/Client/Adapter/Psr18Adapter.php 92.45% <0.00%> (-0.28%) 22.00% <0.00%> (-1.00%)
src/Component/ResponseParser/Spellcheck.php 95.12% <0.00%> (-0.06%) 48.00% <0.00%> (-3.00%)
... and 12 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 7cf99c9...6bdb382. Read the comment docs.

@thomascorthals
Copy link
Member

This will also close #255.

@wickedOne
Copy link
Collaborator Author

hi @mkalkbrenner @thomascorthals @dmaicher

first of all sincere apologies for my long term of absence.
shit happened, i guess for all of us, the last year or so...

i'm closing this one as i've pretty much drawn the conclusion there's no generic way to deal with solr's APIs.

they all have their quirks of which some quite annoying (hello request parameter api 👋 ) and as solarium pretty much already offers solutions to those problems if you're creative, i doubt whether it's desireable to provide an interface to communicate with solrs apis from within this bundle.

i've been messing about with their APIs over here for the last couple of weeks, just for the fun of it. i guess you'll get my point once you browse the source.

needless to say that, if you see anything over there you want to have integrated in this library, just let me know.

w

@wickedOne wickedOne closed this Oct 29, 2021
@wickedOne wickedOne deleted the api-manager branch October 29, 2021 01:34
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.

solr api v2 manager
4 participants