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

Yaml editor gives error when using @..@ placeholders #190

Closed
20Biere opened this issue Jan 24, 2019 · 12 comments
Closed

Yaml editor gives error when using @..@ placeholders #190

20Biere opened this issue Jan 24, 2019 · 12 comments
Milestone

Comments

@20Biere
Copy link

20Biere commented Jan 24, 2019

Look like this STS 3.5 bug should be reopen for STS 4.1.0.201812201347-RELEASE
I simply past the previous description :
When trying to use 'project properties' as described in this section of the Spring Boot documentation, the Yaml editor gives the following error:

found character @ '@' that cannot start any token. (Do not use @ for indentation)

Here is a sample application.yml:

info:
  build:
    artifact: @project.artifactId@
    name: @project.name@
    description: @project.description@
    version: @project.version@

Note that while this syntax error is given, there is no problem actually building and running the project, and the placeholders do work. This seems to be an issue with the Yaml validation not accounting for the @..@ placeholders.

@martinlippert martinlippert transferred this issue from spring-attic/spring-ide Jan 24, 2019
@martinlippert martinlippert added this to the 4.1.2.RELEASE milestone Jan 24, 2019
@kdvolder
Copy link
Member

This seems to be an issue with the Yaml validation not accounting for the @..@ placeholders.

Yes that sounds right. It is not too surprising because it is actually not valid yaml. The only reason this works in a build is that the build replaces the placeholders with something else before the files are used in the build. The editor has no way of knowing that these things will be replaced.

@kdvolder
Copy link
Member

As a workaround for the bug I beleave you can surround the placeholders with quotes "@...@". This avoids the issue that yaml grammar doesn't allow have tokens starting with a '@'. I beleave the quotes will not stop maven from replacing the placeholders so it should work as intended.

@kdvolder
Copy link
Member

kdvolder commented Feb 1, 2019

I've looked up the old ticket and I think it was previously resolved as a "Won't fix".

Found here: spring-attic/spring-ide#167

That ticket shows that an attempt was already made their to somehow avoid the yaml parser trowing an error on this input. But the partial implementation of this was quite complex and the benefit of this complexity seemed not worth it. Seeing as the problem is so easily avoided by simply placing some quotes around the "@...@".

I should also note the linked ticket brough up as a objection to the workaround that it doesn't work when the type of the property is something other than string. For example integeter. The example given was:

server:
  port: @xmd.spring.config.port@

In this example, adding quotes around the "@...@" still ended up with a type error. The editor's checking algorithms was patched in response to this to handle that case as well and not report a error.

I have checked and we in fact still have a regression test for this very example and it still works in STS 4.

So, basically, I think we previously decided that we wouldn't support this and I don't think anything has really changed since then. So I'm inclined to just close this ticket again with the same reasoning.

Feel free to argue otherwise, we can always reconsider. But... I'll probably want to here some compelling arguments why simply putting some double quotes around these placeholders is too much trouble.

@spring-projects-issues
Copy link

(comment in Pivotal Tracker added by Kris De Volder:)

Closing and delivering this ticket as 'Won't fix'. See the github issue for detailed justification.

@20Biere
Copy link
Author

20Biere commented Feb 4, 2019

Hi,
A bit late, was busy ...
Placing quote around the placeholder make the file look weird once filtered by Maven, and may lead to confusion when read. In this example showing filtered value, what is the target properties type?
property1 : "true"
property2: false
property3 : "123"
property4 : 123

The editor should respect the specification. A replacement token should be @...@ and not '@...@' (lol looks like smileys :p )
And it's a bit absurd to not be able to start a token with a @.

@kdvolder
Copy link
Member

kdvolder commented Feb 4, 2019

And it's a bit absurd to not be able to start a token with a @.

It's not really our choice. That's part of the yaml syntax. We are using an 'off the shelf' yaml parser (snakeyaml) so this is rather hard for us to change. The parser simply doesn't like the input and unless we write our own parser, or fork the existing one, this is kind of hard to fix.

I did have an idea last time. Basically, I tried to transform the input a bit before feeding it to the parser (i.e trying to find bits of input that look like '@...@' and replace them with something different. I started implementing that. But I backed out of it when it got rather complex and I felt like I was probably introducing more strange bugs than I as fixing with it.

what is the target properties type?

Spring boot doesn't really care about the types of the values anyways. It has its own parser to parse things like booleans etc. from strings. In fact when you use the equivalent .properties file format all values attached to properties are allways strings. Spring boot will convert them into whatever types it expects. So really, it hardly matters whether you write true or "true". I do agree it looks a bit odd. But functionally it is really completely equivalent.

The editor should respect the specification

What specification? Yaml syntax explicitly forbids a '@' there. So in a sense it does.

BTW: If you don't like "@...@" you can also use ${...} without quotes. I tried that and this works for me (but last time I suggested it as a workaround, someone did raise some potential objections to that workaround as well, so I'm not totally sure how good an idea it is.

Anyhow... I understand your objections to the work around, and indeed, in an ideal world, it shouldn't be necessary. I still don't feel like its worth a complex dirty hack on the editor side, when the workaround is rather trivial. So unless we get some more people complaining about this and coming out to say they really want this fixed, I think we'll leave it as it is for now.

@spring-projects-issues
Copy link

(comment in Pivotal Tracker added by Martin Lippert:)

I am moving this back into the backlog, lets see if more people chime in and provide additional input.

@fischman98
Copy link

Thanks for re-opening this; @spring-issuemaster & @martinlippert. I vote to have this resolved one way or another...if the yaml spec doesn't allow '@' to start values, maybe another, non-conflicting delimiter can be used by Spring. It would be nice to have the "official instructions/documentation" NOT cause a validation error.

@martinlippert
Copy link
Member

@fischman98 I completely agree

@martinlippert
Copy link
Member

I think we need to find a way to somehow ignore those @...@ property values in the editor:

  • putting them in quotes "just to make the editor validation happy" doesn't seem to be the right approach here.
  • changing them from @...@ to {...} changes the semantics as far as I can read from the spring boot documentation.
  • using @...@ for replacing values in the build process with property values from maven is explicitly documented in the spring boot doc, so the editor should support this (at least not show an error).
  • I would not go that far to support content-assist within the @...@ section for properties from Maven build files, but even that would be a nice add-on (for later).

First step here would be to ignore this. As far as I know the validation logic of the editor already ignores those patterns for deep validation. The missing piece is to tell the parser to ignore this as well. Maybe the easy solution here would be to cheat a bit and replace those @...@ sections with "@...@" before doing the parsing, while shortening the ... by two characters to keep the document positions inside of the parsed AST nodes and the real doc aligned. Just an idea. This would break if you would see @.@ with exactly one character as the value to replace between the two @, but that would be acceptable for me.

@kdvolder
Copy link
Member

kdvolder commented May 5, 2020

maybe another, non-conflicting delimiter can be used by Spring

I think this was proposed once already and the spring/boot folks said no. I beleave this is actually configurable by the user themselves somehow as well, so their reasoning was that they won't change the default, but said that if, you, as a individual user, really wanted to, you can change it for your own project. But they were not willing to change the default as that would be too much of a breaking change for a lot of users.

As to @martinlippert suggestion. I went down the path of trying to 'hack' around this by mangling the input already and it's complex and messy. But I suppose could give it another try. Idea of making the substition in such a way that it retains the length might help a bit. Thinking maybe we just replace these things with "@@@@@@@@@@@@@" repeating the '@' as many times as needed to retain the original lenght and then make sure we recognize it as something to ignore completely during validation.

@kdvolder
Copy link
Member

I've pushed the 'fix' based on the idea of replacing stuff that looks like it might be a maven placeholder with "@@@@@@@@@@@@@".

The fix is probably not 100% correct. Making it so would mean accounting for all kinds of weird interactions with yaml and string escape sequences. For example if you actually used the workaround previously recommended like this:

property: "@placeholder@"

Then naive implementation would 'secretly' transform that into:

property: ""@@@@@@@@@@@@""

... and this breaks the parser again (where it wasn't broken before).

Being aware of this particular likely case, I've added an extra check to detect that and leave it untouched. But this is not a 100% correct implementation just a 'quick and dirty' check that makes the known cases / tests pass. A 100% correct implementation would really need to have the same smarts as a full yaml parser.

Anyhow, I think practically speaking this probably works 99% of the time and we can always add a few more cases if people report bugs in this area in future.

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

No branches or pull requests

5 participants