[LiveComponent] Multiple URL query params in one LiveProp DTO

Hello,
Part of the discussion about faceted search menu and results page combo (challenge 1), I'm searching to map different url parameters into one object.
In a symfony controller, you can welcome several query params in a single DTO with the MapQueryString attribute.
I wonder if we can, also in live components, group several query params into one property class?
In a live component, currently, when you have multiple query parameters to welcome, you must declare them one by one; then eventually, in a method, regroup them into an array or a DTO to do some logic. It can be cumbersome if you have a lot of potential query parameters. Plus, and I feel like it's more important, aligning behavior of component controllers (like a LC), with regular controllers is very important as it drastically lowers the mind burden and learning curve.
Context: you got a FacetedSearchMenu component, with multiple parameters that you want to reflect on url, and you group those in a FacetFilter DTO to pass to repository or whatever.
If I may help in any way,
Best regards,

I may add a little bit of context here;
I created 2 issues about a faceted menu/result page. I feel like LC is an awesome tool for this job, but needs 1 or 2 tweaks to do it flawlessly.
I also think a faceted menu example in doc/demo can be a superb vitrine of what LC/UX can do ;)
Please be ensured that is not a "do my homework" post (I felt like when writing) but a real, large opportunity for LCs to be THE solution for any faceted search, as you can find on e-commerce websites/frameworks, for example (my actual usecase is not for shop btw).
Functional specs
From an index page of a given symfony controler, render a template that renders a FacetMenu & an ArticleResults LCs.
The page accepts some query parameters of various nature, like "domain" as a string from db, a "publicationDate" as date, a "name" as a regular search string, a "status" as a string-backed enum, and a classical "page" as an int that reflects pagination.
The Facet Menu got several inputs to play with those values accordingly (with a reset button also). When a value is changed, 2 things happens in realtime:

url is updated with the new parameter's value, allowing user to copy/paste/save this search url,
ArticleResults is updated according to those parameters.
And... That's it. Pretty straightforward, pretty cool, but some challenge are on the (my?) way.

Technical target
I may write some impossible specs below, but at least I'm pretty sure nothing is nonsense here ;) note that below are flawless specs without any workarounds or compromises.

All the url parameters should be received and packed into a Filter DTO, with properties typed accordingly (DateTime, Enum, string, ...), as the MapQueryString feature of Symfony controller does.
empty values (e.g. index?date=&page=1), null values should be allowed (as long as the type allows it in the Filter)
only one read of those url params should be done in the code: the ArticleController:index() should not take card of the url params, but the FacetMenu and the ArticleResults do; The first to set inputs accordingly for UX, the second to display results.
the Filter DTO should be populated from LC with proper types in both LCs,
for the FacetMenu LC, FilterDTO should be a live property, able to read/write params from/to the URL in real time.
for the ArticleResults LC, FilterDTO should be readonly, populated from URL in real time.
I wonder if the FacetMenu should be treated as a Form, as the inputs that re-render the whole act like a form, the url parameters thing is very "action=get"-ish, but I won't call this as a proper form, more a bunch of inputs packed in a LC. That's a genuine web design question! As a 'no' answer could drastically change the process (not using LiveFormTrait..), I will not take it in account. But I feel this must be shared with you.

Challenges
If my specs are OK, I may have found few not-handled-yet challenges for LCs:

Read URL params from a LC in realtime, and populate a Filter DTO as live property based on those params --> allows to get rid of url parameters handling from controler, and not having to pass any prop to the LCs when rendering the first time from template. Also, an important side effect of this is to align behavior from regular SF controler and LCs, that drastically lower the mind burden and learning curve.
Accept 'empty values' of a typed property as a null one --> I have this problem in regular controler: LC produce an url with empty 'date' parameter, and as it is not null, controler tries to transform it as a DateTime and fails, so I handle my date as a string.
Empty/default should not be written as url parameter when LC update URL from Filter DTO --> another way to handle the &date= issue, and clean URL for better UX.
Instead of passing serialized parameters from FacetMenu to ArticleResults when updating a field, just trigger a "rerender" at ArticleResults that will make it read url parameters again, doing logic accordingly (checks, ORM, ...) before re-rendering.

Based on those challenge, issues will be created to address those points, until I totally miss a thing here.

Possibilities:

new #[LiveQueryString] attribute?
make #[mapQueryString] works on a LC (without extending AbstractController?)
make url: true works on a DTO

I'm having  doubt here... are DTO as LiveProp supposed to work already, even with URL ....
https://symfony.com/bundles/ux-live-component/current/index.html#supported-data-types

Interesting point, did you achieve any minimal working example? Because I try since 1hour, and I always throw errors:

An exception has been thrown during the rendering of a template ("Warning: Undefined property: App\Component\ArticleResults::$name").

class ArticleResults
{
    use DefaultActionTrait;
    use ComponentToolsTrait;

//    #[LiveProp(url: true)]
//    public ?string $name = null;
//    #[LiveProp(url: true)]
//    public ?string $status= null;
//    #[LiveProp(writable: true, url: true)]
//    public ?int $page = 1;

    #[LiveProp(writable: true, url: true)]
    public ?Filter $filter = null;

    private ItemsPage $articlesPage;

    public functiondoStuff(): foo {...}
}

class Filter
{
    public function __construct(
        public ?int $page = 1,
        public ?string $name = null,
        public ?string $status = null,
    ) {}
}

I may miss something goofy here, but for me and based also on the DTO part of the doc, I can't make it work 😢

The error message you quote may indicate you try to mount "name" directly on ArticleResults in your template, no ?

https://github.com/user-attachments/assets/75357178-bc62-4d18-945f-2ac18c29e25e

(sorry for the support page ... i was working on something else 😅 )

<?php

namespace App\Twig;

use Symfony\UX\LiveComponent\Attribute\AsLiveComponent;
use Symfony\UX\LiveComponent\Attribute\LiveProp;
use Symfony\UX\LiveComponent\DefaultActionTrait;

#[AsLiveComponent]
final class Search
{
    use DefaultActionTrait;

    #[LiveProp(writable: ['search', 'size', 'type'], fieldName: 's', url: true)]
    public SearchFilters $filters;

    public function mount(): void
    {
        $this->filters ??= new SearchFilters();
    }

    public function getResults(): array
    {
        $results = [
            ['name' => 'Gorilla', 'size' => 'lg',  'type' => 'animal'],
            ['name' => 'Banana', 'size' => 'md',  'type' => 'fruit'],
            ['name' => 'Apple', 'size' => 'sm',  'type' => 'fruit'],
            ['name' => 'Lion', 'size' => 'lg',  'type' => 'animal'],
            ['name' => 'Strawberry', 'size' => 'sm',  'type' => 'fruit'],
            ['name' => 'Elephant', 'size' => 'lg',  'type' => 'animal'],
            ['name' => 'Orange', 'size' => 'md',  'type' => 'fruit'],
            ['name' => 'Tiger', 'size' => 'lg',  'type' => 'animal'],
            ['name' => 'Pineapple', 'size' => 'md',  'type' => 'fruit'],
            ['name' => 'Zebra', 'size' => 'lg',  'type' => 'animal'],
        ];

        $results = array_filter($results, function($result) {
            return $this->filters->search === '' || str_contains($result['name'], $this->filters->search);
        });

        $results = array_filter($results, function($result) {
            return $this->filters->size === '' || $this->filters->size === $result['size'];
        });

        $results = array_filter($results, function($result) {
            return $this->filters->type === '' || $this->filters->type === $result['type'];
        });

        return $results;
    }
}

<?php

namespace App\Twig;

use Symfony\UX\LiveComponent\Attribute\AsLiveComponent;
use Symfony\UX\LiveComponent\DefaultActionTrait;

final class SearchFilters
{
    public string $search = '';

    public string $type = '';

    public string $size = '';
}

dump(this.filters)

    {{ dump(this.filters) }}

Filters

            Search
            <input type="search" class="form-control" data-model="s[search]" />

            Size
            <select class="form-control" data-model="s[size]">
                <option value="">Size</option>
                <option value="sm">Small</option>
                <option value="md">Medium</option>
                <option value="lg">Large</option>
            </select>

            Type

                <input type="radio" class="form-check" data-model="s[type]" value="animal" />
                Animal

                <input type="radio" class="form-check-input" data-model="s[type]" value="fruit" />
                Fruit

Results

        {% for result in this.results %}

{{ result.name }}

                    {{ result.size }}
                    {{ result.type }}

        {% endfor %}

(please do not look too much at the code quality 😆 )

Thank you very much for taking the time to create example, I made some tries and here's my thoughts:

You're right, my error was that I still aimed for this.page instead of this.filter.page!
It also needs a mount() method where we instantiate the DTO: $this->filters ??= new SearchFilters();. Strange thing that we have #[PreMount] & #[PostMount] attributes, but no #[Mount] 🫢
Writable: true doesn't allow the properties inside the Filter dto to be written; you MUST explicitely call the properties in an array.
I may miss a point (again!) but it SEEMS that an 'onChange' event is not triggered when a property inside the DTO changes? Here hello doesn't appears when I try to change anyhow the page value on template:

class FacetMenu
{
    use DefaultActionTrait;

    #[LiveProp(writable: ['page'], onUpdated: 'emitChange', url: true)]
    public Filter $filter;

    public function mount() { $this->filter ??= new Filter; }

    public function emitChange(): void
    {
        dd('hello');
        $this->emit('facetSetted', ['filter' => $this->filter]); // for the next point
    }
}

The ComponentToolsTrait::emit() method only accepts array as $data; so I can't send the SearchFilter object like this. It must be insered into an array like ['filter' => $this->filters], that will be serialized and deserialized as an array. So it must be normalized as a SearchFilter object once in the other component:

class EventResults
{
    use DefaultActionTrait;

    #[LiveProp]
    public Filter $filter;

    #[LiveListener('facetSetted')]
    public function reload(#[LiveArg] array $filter): void
    {
        dd($filter);
    }
}

All those constraints make the move quite complicated; I feel like using a DTO is way less intuitive, but I frankly don't know how to handle those issues 😶

You're very right on all these points.
So, now... would you want to help improving one ? And see where that leads ?
I can help, pin in the direction if you need, and probably help you figuring "what's not gonna stay that way" and "what is needed because this or that features"...
What do you think ?

I definitely accept the challenge of open source!

Adding a #[Mount] attribute: copy/pasting another attribute so trivia I guess (famous last words)
Making #[LiveProp(writable: true)] works on a DTO seems mandatory and hard to counter argue?
Making #[LiveProp(onChange: 'foo')] works on a DTO seems mandatory also?
Having a mount() method to help a DTO to be created is sad, but I guess it's quite tricky to avoid,
#[LiveArg] that could accept an object and auto normalize it, as a regular Symfony controller, is I think, a crossroads. My feeling as a newbie is that the regular SF controllers are very conveniants and intuitives, and the different behavior of LC is suboptimal and creates mind burden. So instead of fixing "LiveArg" I feel like LC should have a 'ControlerToolkit' addon, maybe a trait, or a abstract class, but tweaked for LCs (I know that you can extends AbstractController for a LC, but I guess a random method can't accept params normalization like controllers can), that will help to play with URLs and serlializations, because at the end the day, my whole problems/suggestions are basically "my LC is a little controller that need controller's power, please halp".

Would you agree to take some time to discuss them (slack / meet / anything / ?) when you have the time ?
Just quickly

mount() method is called if exist, so not sure there really is a need here.

I'm certain this will be more complicated than it seems right now... but yeah the DX is not optimal currently. Let's just keep in mind that LiveProp is a "solution" and maybe there is something else to consider.... like "what is a liveprop" and "do we need something else ?

Yes, nothing to think of too much for now, as it entirely depends on the 2

Not sure to understand here? You could instanciate it in the constructor, or before calling the component the first time... .but if you have only an empty class string ... is it really a liveprop you need, or a logger ?

This is were i'm a  bit :-/  A component should only react to a small (limited) set of methods (the actions), value changes, etc... so i would almost say the opposite, what would they use a request  for ? a firewall ? a flash message.
So here we see everyone has its own definition of what is a component, a statefull one, or not :) And i'm really not sure some implementation details we have right now are made for all these different visions. But all of them are legit and we need to clarify how we want to handle them and lower the frustrations.

At the same time of all your very relevant ideas and feedback... I also started to split more clearly Live and Twig component.. so i huess the work on Attributes, Metadata, Events is a must-have to clarify some behaviours, and free some place for new features :)

I would be pleased to speak about this on slack for example; this week my planning is a wreck, I just write issues when I have time, but next week for sure.
Let me argue for #[Mount] as it is very small and simple:

PreMount and PostMount already exists,
It helps code expressivity, as you may want to save one LOC and call your method mount(), but you can also want it expressive, call #[Mount] and create your method 'public function setPropFromRepository()' (ok that's a very bad name),
This SF pattern is common and exists everywhere, for example event listeners let you cann your function 'onFoo()' or let you put an attribute with 'event: foo' and call your method whatever you want,
Note that for completion, I don't know if you can currently create a postMount() function without attribute and make it behave like it has #[PostMount] :)
If it's done, I feel like we can create a small new 'LC events' of documentation, with those 3 and the tricks, make refs from the main page, and begin to slim down the doc 🥳

Point 4, about DTO initialization: I feel like we should left this one behind, unless someone comes with a veeery clever trick.
Maybe the most optimal version will be in the language, being able to write properties like public SearchFilter $filter = new SearchFilter() I don't know if it's packed in 8.4 or will never happen.

Points 2 and 3 about LiveProp, needs to be clear about 5, 'general design' considerations. We agree that DX is not optimal, that TC and LC need more separation, and new features I am requesting (except Mount that is a bit stand alone) must integrate well with general design.
I will take time to read your links about the split, but let me give some food here:

Are you able to find much usecases where a LC does NOT behave like a controller? Because a like/dislike button, a form, a searchbar or an upload field are and behave exactly like a 'micro-controller' (having trouble with this term as it means specifically in IT). My point is: I feel like a LC is deeply related to a controller role, and needs a lot of the regular, powerful SF controller's feature. So I may be biaised...
... But I made some React, and it's a world where 'Components' are not a 'cool feature' but 'THE ONE AND ONLY PARADIGM'.  So my point is that I'm not sure a LC should be only small features, in fact the question is very good: can we give a role to LC that fits for both of us (and for everyone)? I would say packed and independant are the keys, so I agree with 'limited'. You should be able to share a LC between projects, or with friends. I replaced all my stimulus controllers that made ajax calls, and destroyed the src/Controller/AjaxController we all have somewhere. I feel like this is the right use of TC/LCs?
based on previous, maybe the

so i would almost say the opposite, what would they use a request for ? a firewall ? a flash message.

To answer about request, some reasons as I mix good & bad ideas:

to find base URL. Imagine a pagination component, with first/previous/next/last pages. It changes the number of 'page', but what about the baseURL if you use it on different pages? Here you either can import it with a mandatory prop, or deduct it from URL. It improves independency of the the component, at cost of some edge cases where 'no please it's not what I want!'.
to find previous page. Not exactly sure how symfony Security & firewall conf would behave if I make a LC from a login form; When I submit and successfully log in, I would like to be redirected to previous page, and therefor I will want to check in Request if I have the last route in session, or also send the original route with a prop.
ofc to catch URL parameters, until tooling allow to fetch them smoothly :) soon!

You're right for this part, I feel like none of my arguments are killer ones, and a good shaped LC can get rid of any Request need, on classical use cases (if some nerds want to inject it by constructor, so be it).

To be honest, I think we share the same vision on the vast majority of things here... i may have not expressed my personal ideas here with enough talent 😅
The remaining differences are purely philosophical and won't be a problem here :)


Refactors JDBC driver to parse TLS options in VitessConnection class

This is a follow-up to PR #2349, including some refactoring cleanup that was omitted from the original PR because that one had grown excessively large and complex.
In the JDBC driver, responsibility for parsing SSL/TLS-related optional settings now resides in the VitessConnection class.  As part of making that work, the gRPC setup is now triggered by VitessConnection.connect() rather than the constructor method.
@bbeaudreault is the most familiar with the prior Slack discussion leading up to this.

There's some ongoing discussion in Slack, but I would recommend that we increment the version number of this JDBC driver artifact prior to merging this pull request.
It's currently sitting at 1.0-SNAPSHOT, which has been the version number since the code was initially donated from Flipkart.  Even an increment to 1.1.0-SNAPSHOT would be nice, since:

It would bring the convention in line with semver standards, and
There's no easy way to tell which version of the driver artifact you have in hand (and there have been a LOT of changes since the initial Flipkark contribution).  So some new version bump would at least give users a milepost marker to gauge against.

Based on discussion with @harshit-gangal in Slack, I've incremented the version number of the Java client artifacts from 1.0-SNAPSHOT to 1.1.0-SNAPSHOT.  Discussion of a longer-term versioning strategy is ongoing, and outside the scope of this PR.
Because of the way that these Maven build files are structured, bumping the version number requires a search-and-replace that touches multiple files.  That's a bit unfortunate (probably part of the reason why no one's wanted to increase the version number before!), and this could be centralized in a single place with some minor Maven restructuring.  However, that work should probably wait until after more decisions are made regarding the long-term versioning strategy.

Still, the version number should be same for all the artifacts. We should
not differ on that.
On Sun, Feb 5, 2017 at 7:26 PM, Steve Perkins<EMAIL_ADDRESS>wrote:

Based on discussion with @harshit-gangal
https://github.com/harshit-gangal in Slack, I've incremented the
version number of the Java client artifacts from 1.0-SNAPSHOT to
1.1.0-SNAPSHOT. Discussion of a longer-term versioning strategy is
ongoing, and outside the scope of this PR.
Because of the way that these Maven build files are structured, bumping
the version number requires a search-and-replace that touches multiple
files. That's a bit unfortunate (probably part of the reason why no one's
wanted to increase the version number before!), and this could be
centralized in a single place with some minor Maven restructuring. However,
that work should probably wait until after more decisions are made
regarding the long-term versioning strategy.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
https://github.com/youtube/vitess/pull/2529#issuecomment-277521354, or mute
the thread
https://github.com/notifications/unsubscribe-auth/ALSUinWW2ixzzteU8MrJVm4UDmwI_B26ks5rZdUPgaJpZM4L2uGI
.

Reviewed 17 of 17 files at r1.
Review status: all files reviewed at latest revision, 2 unresolved discussions.

java/jdbc/src/main/java/com/flipkart/vitess/jdbc/VitessConnection.java, line 82 at r1 (raw file):

    public void connect() {
        this.vTGateConnections = new VitessVTGateManager.VTGateConnections(this);

What benefit you get to move it to separate method?

java/jdbc/src/main/java/com/flipkart/vitess/jdbc/VitessDatabaseMetaData.java, line 29 at r1 (raw file):

    public String getURL() throws SQLException {
        return this.connection.getUrl().getUrl();

Check for VitessJDBCUrl to be NotNull

Comments from Reviewable

Review status: all files reviewed at latest revision, 3 unresolved discussions.

java/jdbc/src/main/java/com/flipkart/vitess/jdbc/VitessConnection.java, line 82 at r1 (raw file):
Previously, harshit-gangal wrote…
What benefit you get to move it to separate method?

We thought it a bad practice to pass (this) in to another constructor while the object itself was still constructing. I think it should work in our case currently, but may not always be true in the future.

java/jdbc/src/main/java/com/flipkart/vitess/jdbc/VitessJDBCUrl.java, line 163 at r1 (raw file):

        this.trustStore = caseInsensitiveKeyLookup(info, Constants.Property.TRUSTSTORE);
        this.trustStorePassword = caseInsensitiveKeyLookup(info, Constants.Property.TRUSTSTORE_PASSWORD);
        this.trustAlias = caseInsensitiveKeyLookup(info, Constants.Property.TRUST_ALIAS);

I think all of these can be removed now, right? Including the getters you added below as well.

Comments from Reviewable

Review status: all files reviewed at latest revision, 3 unresolved discussions.

java/jdbc/src/main/java/com/flipkart/vitess/jdbc/VitessConnection.java, line 82 at r1 (raw file):
Previously, bbeaudreault (Bryan Beaudreault) wrote…
We thought it a bad practice to pass (this) in to another constructor while the object itself was still constructing. I think it should work in our case currently, but may not always be true in the future.

ok.

java/jdbc/src/main/java/com/flipkart/vitess/jdbc/VitessJDBCUrl.java, line 163 at r1 (raw file):
Previously, bbeaudreault (Bryan Beaudreault) wrote…
I think all of these can be removed now, right? Including the getters you added below as well.

This the only place where the URL is parsed. If I am right?

Comments from Reviewable

Review status: all files reviewed at latest revision, 3 unresolved discussions.

java/jdbc/src/main/java/com/flipkart/vitess/jdbc/VitessJDBCUrl.java, line 163 at r1 (raw file):
Previously, harshit-gangal wrote…
This the only place where the URL is parsed. If I am right?

The URL is parsed at https://github.com/youtube/vitess/blob/34a35fc2baec8a440a64a4445fd0508670167044/java/jdbc/src/main/java/com/flipkart/vitess/jdbc/VitessJDBCUrl.java#L260 and all values are placed into the Properties object.
The Properties object is returned and used in ConnectionProperties. Accessing of properties should happen in ConnectionProperties.
Basically the goal here was to make VitessJDBCUrl be used for generic parsing of URL parameters into Properties key/values, merging with existing properties where appropriate. I left the host/user/password in there because they were part of the url structure (not parameters). But they also could be moved if we wanted.
All reading of configuration parameters should happen in ConnectionProperties. This provides a centralized place where you can see all the properties, their descriptions, their default values, and whether they are required.
@steve-perkins did part of that, moving the getters into ConnectionProperties.

Comments from Reviewable

The group discussed it Slack yesterday (2017-03-02, 2:00 PM EST), and the consensus was to bump it to 1.1.0-SNAPSHOT for simplicity right now.  Longer term, a strategy is desired to align the JDBC driver version with the overall Vitess version number, and move to release rather than snapshot versions.

Review status: all files reviewed at latest revision, 3 unresolved discussions, some commit checks failed.

Comments from Reviewable

Review status: all files reviewed at latest revision, 3 unresolved discussions, some commit checks failed.

java/jdbc/src/main/java/com/flipkart/vitess/jdbc/VitessDatabaseMetaData.java, line 29 at r1 (raw file):
Previously, harshit-gangal wrote…
Check for VitessJDBCUrl to be NotNull

Done.

Comments from Reviewable

Reviewed 17 of 17 files at r1, 5 of 5 files at r2, 1 of 1 files at r3, 1 of 1 files at r4, 1 of 1 files at r5, 1 of 1 files at r6.
Review status: all files reviewed at latest revision, 5 unresolved discussions.

java/example/pom.xml, line 17 at r1 (raw file):

      <groupId>com.flipkart.vitess</groupId>
      <artifactId>vitess-connector-java</artifactId>
      <version>1.1.0-SNAPSHOT</version>

I'm not going to insist on this for now -- @michael-berlin can handle it when he does the eventual real versioning -- but I think it would be best to make version a property:
<properties>
  <vitess.version>1.1.0-SNAPSHOT<vitess.version>
</properties>

Then in all of these poms you can just make it look like this:
<dependency>
      <groupId>com.youtube.vitess</groupId>
      <artifactId>client</artifactId>
      <version>${vitess.version}</version>
 </dependency>

Will result in less changes in the future.  The one place I don't think this works is in the  block. But also we don't really need to bump the parent version unless we change the packaging, so not a huge issue. We can just bump all the child versions when we want to.

java/jdbc/src/main/java/com/flipkart/vitess/jdbc/ConnectionProperties.java, line 52 at r1 (raw file):

    // TLS-related configs
    private BooleanConnectionProperty useSSL = new BooleanConnectionProperty(

This is more of just a question, potentially out of scope for this PR:
Recently I was looking at the basic (non-vitess) JDBC SSL configs and saw the verifyServerCertificate option. Should we be supporting that here? It seems like a useful option for situations where you want encryption but trust your network.

Comments from Reviewable

I added 1 optional comment and had a question for you. Thanks for the cleanup!

Review status: all files reviewed at latest revision, 5 unresolved discussions.

Comments from Reviewable

Review status: 17 of 18 files reviewed at latest revision, 5 unresolved discussions.

java/example/pom.xml, line 17 at r1 (raw file):
Previously, bbeaudreault (Bryan Beaudreault) wrote…
I'm not going to insist on this for now -- @michael-berlin can handle it when he does the eventual real versioning -- but I think it would be best to make version a property:
<properties>
  <vitess.version>1.1.0-SNAPSHOT<vitess.version>
</properties>

Then in all of these poms you can just make it look like this:
<dependency>
      <groupId>com.youtube.vitess</groupId>
      <artifactId>client</artifactId>
      <version>${vitess.version}</version>
 </dependency>

Will result in less changes in the future.  The one place I don't think this works is in the  block. But also we don't really need to bump the parent version unless we change the packaging, so not a huge issue. We can just bump all the child versions when we want to.

Agreed (although yeah, I would prefer that this be part of the ultimate "real versioning" effort).

java/jdbc/src/main/java/com/flipkart/vitess/jdbc/ConnectionProperties.java, line 52 at r1 (raw file):
Previously, bbeaudreault (Bryan Beaudreault) wrote…
This is more of just a question, potentially out of scope for this PR:
Recently I was looking at the basic (non-vitess) JDBC SSL configs and saw the verifyServerCertificate option. Should we be supporting that here? It seems like a useful option for situations where you want encryption but trust your network.

Sure.  I've never used this option personally, but I see no reason for Vitess not to support it.  As long as this particular pull request has lingered around though, I'd rather see it done is a separate PR.

java/jdbc/src/main/java/com/flipkart/vitess/jdbc/VitessJDBCUrl.java, line 163 at r1 (raw file):
Previously, bbeaudreault (Bryan Beaudreault) wrote…
The URL is parsed at https://github.com/youtube/vitess/blob/34a35fc2baec8a440a64a4445fd0508670167044/java/jdbc/src/main/java/com/flipkart/vitess/jdbc/VitessJDBCUrl.java#L260 and all values are placed into the Properties object.
The Properties object is returned and used in ConnectionProperties. Accessing of properties should happen in ConnectionProperties.
Basically the goal here was to make VitessJDBCUrl be used for generic parsing of URL parameters into Properties key/values, merging with existing properties where appropriate. I left the host/user/password in there because they were part of the url structure (not parameters). But they also could be moved if we wanted.
All reading of configuration parameters should happen in ConnectionProperties. This provides a centralized place where you can see all the properties, their descriptions, their default values, and whether they are required.
@steve-perkins did part of that, moving the getters into ConnectionProperties.

Done.

Comments from Reviewable

LGTM.
@michael-berlin I'm going to be away when  you do the io.vitess refactor. Can you also at that time do the change I suggested above? That is, use a property for the version instead of hardcoding it in every pom multiple times
@steve-perkins agreed, verifyServerCertificate can be a separate PR just wanted to see if there was a reason.

@michael-berlin I'm going to be away when you do the io.vitess refactor. Can you also at that time do the change I suggested above? That is, use a property for the version instead of hardcoding it in every pom multiple times

Ack.

Okay, all comments have been addressed, git commits squashed, and Travis CI is green.  No further changes are planned on my end unless anyone has anything further.

Review status: 17 of 18 files reviewed at latest revision, 5 unresolved discussions.

Comments from Reviewable

LGTM since @bbeaudreault signed off on the change.
Merging now.


Project.documentation_type is auto

We have 528 projects with the Project.documentation_type set in auto and this value is not allowed in the code.
This value is used, for example at: https://github.com/rtfd/readthedocs.org/blob/be4ff5b9c4fb6f8c4f8b093e98a1359491a52b1f/readthedocs/doc_builder/python_environments.py#L276
But the dictionary used to get the proper builder class doesn't have the auto value and make the build to fail:
https://github.com/rtfd/readthedocs.org/blob/be4ff5b9c4fb6f8c4f8b093e98a1359491a52b1f/readthedocs/doc_builder/loader.py#L15-L32
We do have a way to protect ourselves for this, but this is still happening in production.
https://github.com/rtfd/readthedocs.org/blob/be4ff5b9c4fb6f8c4f8b093e98a1359491a52b1f/readthedocs/projects/tasks.py#L429-L431
I suppose that this is still happening because of #3946 isn't deployed yet.

This doesn't seem to be too important, but just filling this issue to keep track of this and make sure that it doesn't happen anymore after that PR is deployed.

Ref: https://sentry.io/read-the-docs/readthedocs-org/issues/472212666/events/latest/

Sentry issue: READTHEDOCS-ORG-CWX

Lets target this for this release, I'm seeing a lot of new errors around this project type, including ^

To reproduce this, just change the Documentation Type to auto and the next build will fail.


Create CODE_OF_CONDUCT.md

Suggested code of conduct template by Github applied to our project. Reporting procedures made such that it is know who will review any reports and where reports against those individuals can be filed.

Thanks


Selection and Distances don't seem to work

Hi @harveyslash
First of all thanks for giving detailed documentation to this project, it's really helpful. I'm trying to replicate this on my localhost:8000 without any changes.  I'm wondering why the selections (click actions on the sprites images) functionality doesn't seem to work. I don't get any error while hosting as well.
Anyway to troubleshoot this. Any help appreciated.
Mohammed Ayub

Unfortunately, the projector mechanic itself is now part of this project. If you can tell me the process to reproduce the problem, I may file a bug at the projector project.


Update Webrick

Which issue(s) this PR fixes:
Fixes #4060
What this PR does / why we need it:
This PR allows us to use the latest Webrick v1.8.
https://github.com/ruby/webrick/releases
Without this fix,  plugins using the latest version of Webrick tries to bundle the Fluentd v1.12.1.

https://github.com/fluent-plugins-nursery/fluent-plugin-utmpx/issues/6

Docs Changes:
Not needed.
Release Note:
Not needed.

TODO confirm test results.

All Windows tests fail:
"fatal: not enough memory for initialization

The fluentd command fails by
[BUG] transient_heap_block_alloc: failed

All Windows tests fail:
"fatal: not enough memory for initialization

The fluentd command fails by
[BUG] transient_heap_block_alloc: failed

These tests have failed before as well.
In my local Ruby3.1 on Windows 10 Home, all tests in TestFluentdCommand is successful.

I will run all tests in my local Ruby3.2 on Windows 10 Home.

I have confirmed all tests are successful on my local environment.

Windows 10 Home
Ruby 3.2

I rerun CI for this PR a few times but always fail on Windows platform.
Something might be broken on Windows.

Thanks for confirming!
Indeed, it looks like something broken...
I will investigate it.

I'm not sure what was happening, but now this seems to be working with no problem.

Thanks!

Thanks for your review!


[bug]: bunx --bun shadcn-ui@latest add table

Describe the bug
ConnectionClosed: The socket connection was closed unexpectedly. For more information, pass verbose: true in the second argument to fetch()
path: "https://ui.shadcn.com/registry/index.json"
Failed to fetch components from registry.
Affected component/components
table
How to reproduce
bunx --bun shadcn-ui@latest add table
Codesandbox/StackBlitz link
No response
Logs
No response
System Info
macOS Monterey 12.7.4

Before submitting

[X] I've made research efforts and searched the documentation
[X] I've searched for existing issues

node -v
v20.10.0
bun -v
1.1.4

I got similar error. I accidentally ran yarn install in my bun project folder which added "packageManager": "..." into package.json and forgot about it.
This confused package manager detection and bunx --bun shadcn-ui@latest add ... command then uses yarn package manager.
I fixed it by removing all yarn mentions (yarn.lock, packageManager in package.json, ...) and re-run bunx --bun shadcn-ui@latest add ....


ensureCacheSpaceWithLock panics with variable sized keys

If variable sized keys are being used, then ensureCacheSpaceWithLock() can panic during the cache cleanup routine which loops through safe(). This occurs if the key being inserted is larger than the last key removed.
It would be better if either this key were inserted anyway (and thus exceeding the cache threshold) or the routine was modified so that it cleared some percent of the total cache size before proceeding.

I've fixed or improved a number of things related to this in my fork:

If ensureCacheSpaceWithLock removes the same key that it adds, the cache size is incorrectly calculated.

Updating a key with a smaller sized one incorrectly updates the cache size.

ensureCacheSpaceWithLock now clears out 15% of the cache when it exceeds the limit. Previously, it would clear just enough space to bring it under the limit, and the cache would fill up on the next new key. This causes a lot of CPU thrashing.

Nice! Would love to see a PR with these fixes (in isolation).

Thanks !

I got bitten by a panic in ensureCacheSpaceWithLock over the weekend, although I'm not clear what I did to trigger it. Any thoughts on building a PR? I look at look at WinstonPrivacy's fork but I'm afraid I can't be sure exactly which commits are relevant to this issue and which are for other things.

On further review, I'm not convinced that WinstonPrivacy's fixes actually do anything, but I'm also not seeing any way for the cache size to get out of sync.
When Write is called, any existing value is blown out of the cache, with the cache size decremented by whatever was actually in the cache map.
Whenever we read an item and add it to the cache, it increments the cacheSize by whatever the size of the actual value may be.
I don't see a way in which we could, say, push a 100 byte value into the cache but increment the cacheSize by 200.
I've fooled around with tests to try and replicate the crash, but I cannot. I don't know how this happened:
panic: 833478 bytes still won't fit in the cache! (max 1048576 bytes)

goroutine 603606 [running]:
github.com/peterbourgon/diskv.(*Diskv).ensureCacheSpaceWithLock(0xc0001413b0,
0xcb7c6, 0x1eb2580, 0xc000d09588)
        github.com/peterbourgon/diskv@v2.0.1+incompatible/diskv.go:611 +0x2bd
github.com/peterbourgon/diskv.(*Diskv).cacheWithLock(0xc0001413b0,
0xc001933b60, 0x26, 0xc0170ae000, 0xcb7c6, 0xf9f82, 0xc000074060,
0x1001)
        github.com/peterbourgon/diskv@v2.0.1+incompatible/diskv.go:532 +0x3f
github.com/peterbourgon/diskv.(*Diskv).cacheWithoutLock(0xc0001413b0,
0xc001933b60, 0x26, 0xc0170ae000, 0xcb7c6, 0xf9f82, 0x0, 0x0)
        github.com/peterbourgon/diskv@v2.0.1+incompatible/diskv.go:550 +0xdc
github.com/peterbourgon/diskv.(*siphon).Read(0xc014d6b170,
0xc0019f0000, 0x1000, 0x1000, 0x778, 0x0, 0x0)
        github.com/peterbourgon/diskv@v2.0.1+incompatible/diskv.go:386 +0x12f
bufio.(*Reader).fill(0xc01cf6eea0)
        bufio/bufio.go:100 +0x103
bufio.(*Reader).ReadSlice(0xc01cf6eea0, 0x30000000000a, 0x158, 0x160,
0xc01474d080, 0x0, 0x0)
        bufio/bufio.go:359 +0x3d
encoding/csv.(*Reader).readLine(0xc000d09c38, 0x16, 0x8, 0xc01474d080,
0xc01458db00, 0x203000)
        encoding/csv/reader.go:218 +0x49
encoding/csv.(*Reader).readRecord(0xc000d09c38, 0x0, 0x0, 0x0, 0x1,
0xc01fecce18, 0x4, 0xc01fecce22, 0x7)

This was running on v2.0.1 but I don't see any substantive changes between then and HEAD in terms of the cache.