Fix regex pattern for character conversion

[mabezi]

Related Issue

#1404

New Behavior

Correct replacement of dashes and dots in slugs.

Contrast to Current Behavior

Incorrect replacement leads to errors when using slugs with multiple consecutive dashes.

Discussion: Benefits and Drawbacks

This would make it possible to use tags like customer--123 without errors.
There are also open source projects (e.g. Yaook) that frequently use multiple consecutive dashes in Netbox tags (yaook--default-gateway).

At this point, however, I would like to question why dashes are replaced in slugs at all, since dashes are valid characters in slugs.

Changes to the Documentation

not needed

Proposed Release Note Entry

Fixed regex pattern for character conversion when using slug parameter in multiple modules

Double Check

• I have read the comments and followed the CONTRIBUTING.md.
• I have explained my PR according to the information in the comments or in a linked issue.
• My PR targets the devel branch.

[sc68cal]

At this point, however, I would like to question why dashes are replaced in slugs at all, since dashes are valid characters in slugs.

So, it may be that the issue of Ansible disallowing hyphens for group names, may have accidentally been applied to slugs as well. I agree, the documentation for slug fields explicitly permits hyphens.

So really, I have to wonder what exactly this was trying to prevent? Was it meant to just protect people from adding too many hyphens? I wonder if we shouldn't just remove this part completely?

Should it just be

        return re.sub(r"[^\-\.\w\s]", "", value)

?

[mabezi]

Are you sure that this would cover all valid cases?

I hadn't thought about it before, but the regex allows periods, which should actually be prohibited.
It also doesn't accept underscores, which are definitely allowed.

A slug is a short label for something, containing only letters, numbers, underscores or hyphens.

[sc68cal]

I wasn't sure, so I went and looked. I think we need to update this function to be more aligned with what NetBox does in their code, see my comment

[mabezi]

So, should we just copy the regular expressions from the Netbox project?

My honest opinion:
Depending on the use case, I can also imagine not running any filter at all. Worst case no result is returned by the API.

Detailed input validation is not always necessary in my opinion, because things like regular expressions can diverge after some time.
Especially when it comes to tasks where I just assign a tag, the developer should learn to make correct inputs.

But I can also simply adjust the regular expressions :)

[sc68cal]

I think at the very least we should copy the regex expressions from the Netbox project. However, if the NetBox API gives us back a useful error code about the slug not passing validation, then I could be convinced to drop validation on our side completely

[sc68cal]

Ok, I am 👍 on us removing our regex logic client side

[mabezi]

This seems to be the source of this message:
https://github.com/django/django/blob/main/django/core/validators.py#L293-L301

(But I'm not 100% sure tbh)

[mabezi]

I will make the removal (maybe tomorrow?) and try to test all of the affected functions (as far as possible).

[mabezi]

A small side note: The regex logic cannot be completely removed at the moment. Correcting the incorrect regular expressions still appears necessary.

Reason:

ansible_modules/docs/getting_started/how-to-use/modules.rst

Lines 120 to 141 in 6b9cc00

	Along with forcing a user to provide some uniqueness to their objects in NetBox, we also try and mirror the module interaction with the GUI interaction where we can to prevent burdening the user.
	For instance, the ``slug`` field is required when interacting with the API for the majority of models in NetBox, but constructing the ``slug`` is handled for the user within the GUI. To stay aligned with the GUI,
	we abstract that away from the user by constructing the ``slug`` from the ``name`` using the same rules as the NetBox GUI.
	For reference, here is the code that **slugifies** the ``name`` argument when a user does not provide a ``slug``.
	.. code-block:: python
	def _to_slug(self, value):
	"""
	:returns slug (str): Slugified value
	:params value (str): Value that needs to be changed to slug format
	"""
	if value is None:
	return value
	elif isinstance(value, int):
	return value
	else:
	removed_chars = re.sub(r"[^\-\.\w\s]", "", value)
	convert_chars = re.sub(r"[\-\.\s]+", "-", removed_chars)
	return convert_chars.strip().lower()

The automatic generation of a slug when creating tags etc. is explicitly intended as a feature.
Removing this feature might be a disproportionately large change. (Or what do you think about it?

[sc68cal]

No I had not considered that. I think that we'll have to go with the first suggestion of lining up the regex to match what NetBox's front end does. The regex pattern has never changed, it was just slightly incorrect, so I think we can just fix the problem and we'll be ok.

So to whit - we do not need to replace multiple -'s with a single. Just do what NetBox does. Same pattern.

[sc68cal]

I'm digging into this a bit more, I used git blame to look up #101 which references #95, where they have a comment where they look at how the Netbox frontend javascript handles creating slugs.

The code has obviously changed since then, but doing a search I came across https://github.com/netbox-community/netbox/blob/f0507d00bfd077755619b420eae8a5d8dc979d94/netbox/project-static/src/buttons/reslug.ts#L10 which has the code, but also does not appear to trim the number of hyphens.

So, I think we should take the opportunity to make _to_slug reflect what NetBox does, which I believe can be done per the suggestion I put inline.

[mabezi]

I’ve now given this a lot of thought and considered how best to solve it.

Simply adjusting the regular expressions one-to-one isn't sufficient, because at the moment slugs are being replaced even when one is explicitly provided. Completely removing the function doesn’t make sense either, since generating slugs from the name is a desired feature.

This led to the following solution:

• I adapted the regular expressions used for generating slugs in NetBox.
• If a slug is provided as a parameter, the existing slug is preserved and not overwritten.

[mabezi]

The unit tests do not agree with my solution yet. Well 🤨

[mabezi]

Fixed & rebased to devel.

The CI errors that still occur should have nothing to do with my changes.

[sc68cal]

CI tests are green on devel branch and other PRs don't have this error. I think unfortunately these changes are causing a regression and needs to be investigated.

[mabezi]

I have found my mistake in thinking and developed a new plan!

From now on it will be like this:

• The regex is adapted to Netbox source -> Important: In rare cases, this could possibly lead to deviations in how individual slugs are generated! (example: .Hello World. is now hello-world instead of -hello-world- -> maybe we want to highlight this in changelog?)
• netbox_secret does not use character conversion for slug if there is a slug provided
• If a slug is specified as a parameter, no character conversion is performed. -> If someone relies on the slug parameter being automatically processed with _to_slug, this will now lead to a different result.

A very simplified example:

tasks:
    - name: Create IP address within NetBox with only required information
      netbox.netbox.netbox_ip_address:
          netbox_url: http://netbox.local
          netbox_token: thisIsMyToken
          data:
              address: 192.168.1.10
              tags:
                  - "This Tag will be replaced with proper slug"
          state: present

    - name: Create IP address within NetBox with only required information
      netbox.netbox.netbox_ip_address:
          netbox_url: http://netbox.local
          netbox_token: thisIsMyToken
          data:
              address: 192.168.1.10
              tags:
                  - slug: "this-tag-wont-be-replaced-with-proper-slug"
          state: present

This should provide maximum compatibility.

[sc68cal]

Great job, I know that this has turned into a pretty difficult thing to manage, since you have to deal with a lot of corner cases. I think it's a good idea to try and document this new behavior as much as possible as a changelog fragment.

For the most part when I use NetBox, the slugs were generated either by NetBox via the web application and the slugs are just used in my Ansible playbooks to properly reference things after the fact. My hope is that most people are fetching the slug values and just using them, and aren't really depending on the specific corner case behaviors that you have described. That should hopefully take some of the pressure off.

I could be wrong, but that's at least my hope.

I'll do a deeper dive on your work at a later time so I'm fully up to speed with you, but great job

[sc68cal]

This looks good, I just want a bit more time to fully understand before merging

[mabezi]

Is it already clear whether the patch is included with the next version? :)

Thanks in advance.

[sc68cal]

Some thoughts after looking a bit deeper.

[sc68cal]

Small nitpick